Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

properly handle unix read on directory #4649

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

noboruma
Copy link
Contributor

@noboruma noboruma commented Dec 4, 2024

On linux systems the syscall.Read returns -1 in case the read is being performed on a directory.
This does not play well with the documentation of this function which states that n matches the number of bytes count being read. As a result, other code parts may panic, like here:
https://github.com/noboruma/tinygo/blob/fix-os-unix-read/src/os/file.go#L471

The proposition here is to simply set n to 0 in case of directory EISDIR error.

Comment on lines 100 to 104
// In case of EISDIR, n == -21.
// This breaks the assumption that n always represent the number of read bytes.
if err == syscall.EISDIR {
n = 0
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused by this change. I tried 1 to recreate your described behavior on my linux machine but could not observe, that the read syscall would return -EISDIR for the bytes read when this error is encountered.

Copy link
Member

@aykevl aykevl Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect n should be set to 0 for any error. actually probably not, there's also EOF.
This might actually be a bug in the Syscall function (implemented in the compiler).

Copy link
Contributor Author

@noboruma noboruma Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using llvm-18 on debian 12 (kernel 6.1)
@leongross on my machine running your C code shows bytes_read = -1, with both gcc 12.2 and clang-18.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated the commit to mention -1 instead of -21.
Fix stays the same, in case of EISDIR we can force n = 0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the proper thing to do here is to check the err returned in https://github.com/noboruma/tinygo/blob/fix-os-unix-read/src/os/file.go#L471 and then abort the operation. If there really is an error, we should abort anyway, so the value of n should not be read anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leongross we can, this was actually the first fix I did.
However this code is a copy-paste from official Golang source (GC), so any modification here means we are forking away from std. In std library they handle the n issue at a lower layer, as proposed here.

@noboruma noboruma requested a review from aykevl December 7, 2024 02:25
@deadprogram deadprogram added this to the 0.35.0 milestone Dec 19, 2024
@deadprogram
Copy link
Member

Thank you @noboruma for the code changes and to @leongross and @aykevl for review. Now merging.

@deadprogram deadprogram merged commit eeba90f into tinygo-org:dev Dec 19, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants