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

os: ReadAt should check bounds before re-slicing destination buffer #45592

Open
wdamron opened this issue Apr 15, 2021 · 5 comments
Open

os: ReadAt should check bounds before re-slicing destination buffer #45592

wdamron opened this issue Apr 15, 2021 · 5 comments

Comments

@wdamron
Copy link

@wdamron wdamron commented Apr 15, 2021

What version of Go are you using (go version)?

$ go version
go version go1.15.5 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOOS=linux
GOARCH=amd64

What did you do?

Called the (*os.File).ReadAt method with a 2-byte destination slice.

What did you expect to see?

Data in the destination slice and no error or panic.

What did you see instead?

panic: runtime error: slice bounds out of range [4:2]
goroutine 63 [running]:
os.(*File).ReadAt(...)
    /usr/local/go/src/os/file.go:140 +0x3e5

The panic corresponds with the source line b = b[m:].
Before re-slicing the destination buffer, a bounds-check is needed. Apparently, it's possible for lower layers to return more than the requested number of bytes within the initial read.

@randall77
Copy link
Contributor

@randall77 randall77 commented Apr 15, 2021

it's possible for lower layers to return more than the requested number of bytes within the initial read.

That should never happen. At least, on darwin/amd64 it bottoms out at the pread system call. If it is returning a number greater than the length of the byte slice, then it is probably writing off the end of the byte slice into unknown memory.

Can you provide a reproducible example?

In any case, the fix is not to change the re-slice. It is to fix the underlying bug (which is much more serious, if that is in fact what is happening).

Loading

@wdamron
Copy link
Author

@wdamron wdamron commented Apr 15, 2021

The underlying bug is a DMA kernel driver (exposing a character-device file) which is overriding the requested read/write length for pread/pwrite system calls to be a 32-bit multiple, and then returning the 32-bit multiple as the number of bytes read/written.

The bug is more serious than a missing bounds-check in the ReadAt method, though it may be helpful to panic with a message indicating the read/write length does not match the buffer length. Returning an error in that case might not be the safest option since there's likely memory corruption as a result of the operation, and the documentation for ReadAt only states a non-nil error is returned when n < len(b) (not greater).

Loading

@wdamron
Copy link
Author

@wdamron wdamron commented Apr 15, 2021

I also falsely added GOOS=darwin to this issue report. The issue is seen on linux/amd64. I'll update the description.

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Apr 15, 2021

I'm not sure it's reasonable for the Go standard library to verify that kernel system calls behave correctly. That seems like a fair amount of work for essentially zero gain.

Loading

@networkimprov
Copy link

@networkimprov networkimprov commented May 15, 2021

@gopherbot remove WaitingForInfo

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants