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: File.Seek on Windows errors on offsets like 4G-1, 8G-1, 16G-1 #21681

Closed
gilbertchen opened this Issue Aug 29, 2017 · 14 comments

Comments

Projects
None yet
10 participants
@gilbertchen

gilbertchen commented Aug 29, 2017

Please answer these questions before submitting your issue. Thanks!

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

1.8.1

Does this issue reproduce with the latest release?

I didn't retry, but by looking at the relevant code on the master branch it looks like the bug is still there.

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

Windows 64 bit

What did you do?

Create a 4GB sparse file by moving the file pointer to before the last byte and then write a \x00

Code at:
https://play.golang.org/p/kzMJvpa_eJ

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

What did you expect to see?

It should create the file and returns with success

What did you see instead?

Instead it returns an invalid argument error

This is because SetFilePointer did not handle the return value correctly. According to Microsoft's doc:

Note Because INVALID_SET_FILE_POINTER is a valid value for the low-order DWORD of the new file pointer, you must check both the return value of the function and the error code returned by GetLastError to determine whether or not an error has occurred. If an error has occurred, the return value of SetFilePointer is INVALID_SET_FILE_POINTER and GetLastError returns a value other than NO_ERROR. For a code example that demonstrates how to check for failure, see the Remarks section in this topic.

SetFilePointer in Go always returns the error if you pass 4G - 1, 8G - 1, 16G - 1, etc to the Seek function, even if the operation does return successfully.

@mvdan mvdan changed the title from func (*File) Seek always returns an error on offsets such as 4G - 1, 8G -1, 16G -1 to os: File.Seek on Windows errors on offsets like 4G-1, 8G-1, 16G-1 Aug 29, 2017

@mvdan

This comment has been minimized.

Show comment
Hide comment
@mvdan

mvdan Aug 29, 2017

Member

Seems like the fix plus a test would be rather simple - would you like to give it a try? See https://golang.org/doc/contribute.html

Member

mvdan commented Aug 29, 2017

Seems like the fix plus a test would be rather simple - would you like to give it a try? See https://golang.org/doc/contribute.html

@gilbertchen

This comment has been minimized.

Show comment
Hide comment
@gilbertchen

gilbertchen Aug 29, 2017

@mvdan I can try, but it may take a few days.

gilbertchen commented Aug 29, 2017

@mvdan I can try, but it may take a few days.

@heinrich5991

This comment has been minimized.

Show comment
Hide comment
@heinrich5991

heinrich5991 Aug 29, 2017

SetFilePointerEx seems to be the more appropriate function here, it's even suggested in the docs of SetFilePointer for 64 bit seeks.

This function stores the file pointer in two LONG values. To work with file pointers that are larger than a single LONG value, it is easier to use the SetFilePointerEx function.

heinrich5991 commented Aug 29, 2017

SetFilePointerEx seems to be the more appropriate function here, it's even suggested in the docs of SetFilePointer for 64 bit seeks.

This function stores the file pointer in two LONG values. To work with file pointers that are larger than a single LONG value, it is easier to use the SetFilePointerEx function.

@dsnet dsnet added this to the Go1.10 milestone Sep 1, 2017

@odeke-em

This comment has been minimized.

Show comment
Hide comment
@odeke-em

odeke-em Dec 6, 2017

Member

Gentle ping, how's it going @gilbertchen and @mvdan?

Member

odeke-em commented Dec 6, 2017

Gentle ping, how's it going @gilbertchen and @mvdan?

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Dec 6, 2017

Member

I tried the trivial change, like:

diff --git a/src/syscall/syscall_windows.go b/src/syscall/syscall_windows.go
index 21d5ecf..e6ceb9c 100644
--- a/src/syscall/syscall_windows.go
+++ b/src/syscall/syscall_windows.go
@@ -9,6 +9,7 @@ package syscall
 import (
        errorspkg "errors"
        "internal/race"
+       "runtime"
        "sync"
        "unicode/utf16"
        "unsafe"
@@ -332,6 +333,16 @@ func Write(fd Handle, p []byte) (n int, err error) {
 
 var ioSync int64
 
+var procSetFilePointerEx = modkernel32.NewProc("SetFilePointerEx")
+
+func setFilePointerEx(handle Handle, distToMove int64, newFilePointer *int64, whence uint32) error {
+       _, _, e1 := Syscall6(procSetFilePointerEx.Addr(), 4, uintptr(distToMove), uintptr(unsafe.Pointer(newFilePointer)), uintptr(whence), 0, 0, 0)
+       if e1 != 0 {
+               return errnoErr(e1)
+       }
+       return nil
+}
+
 func Seek(fd Handle, offset int64, whence int) (newoffset int64, err error) {
        var w uint32
        switch whence {
@@ -342,13 +353,17 @@ func Seek(fd Handle, offset int64, whence int) (newoffset int64, err error) {
        case 2:
                w = FILE_END
        }
-       hi := int32(offset >> 32)
-       lo := int32(offset)
        // use GetFileType to check pipe, pipe can't do seek
        ft, _ := GetFileType(fd)
        if ft == FILE_TYPE_PIPE {
                return 0, ESPIPE
        }
+       if runtime.GOARCH == "amd64" {
+               err = setFilePointerEx(fd, offset, &newoffset, w)
+               return
+       }
+       hi := int32(offset >> 32)
+       lo := int32(offset)
        rlo, e := SetFilePointer(fd, lo, &hi, w)
        if e != nil {
                return 0, e

But that doesn't work. Looks like SetFilePointerEx comes with a bunch of warnings about multi-threaded programs and overlapped I/O. Not sure whether those apply.

Also, that GOARCH == "amd64" check is bogus. This should also work on 386 but I don't know how to shove a LARGE_INTEGER through syscall.Syscall6 on Windows.

Maybe @alexbrainman is interested in looking into this?

Member

bradfitz commented Dec 6, 2017

I tried the trivial change, like:

diff --git a/src/syscall/syscall_windows.go b/src/syscall/syscall_windows.go
index 21d5ecf..e6ceb9c 100644
--- a/src/syscall/syscall_windows.go
+++ b/src/syscall/syscall_windows.go
@@ -9,6 +9,7 @@ package syscall
 import (
        errorspkg "errors"
        "internal/race"
+       "runtime"
        "sync"
        "unicode/utf16"
        "unsafe"
@@ -332,6 +333,16 @@ func Write(fd Handle, p []byte) (n int, err error) {
 
 var ioSync int64
 
+var procSetFilePointerEx = modkernel32.NewProc("SetFilePointerEx")
+
+func setFilePointerEx(handle Handle, distToMove int64, newFilePointer *int64, whence uint32) error {
+       _, _, e1 := Syscall6(procSetFilePointerEx.Addr(), 4, uintptr(distToMove), uintptr(unsafe.Pointer(newFilePointer)), uintptr(whence), 0, 0, 0)
+       if e1 != 0 {
+               return errnoErr(e1)
+       }
+       return nil
+}
+
 func Seek(fd Handle, offset int64, whence int) (newoffset int64, err error) {
        var w uint32
        switch whence {
@@ -342,13 +353,17 @@ func Seek(fd Handle, offset int64, whence int) (newoffset int64, err error) {
        case 2:
                w = FILE_END
        }
-       hi := int32(offset >> 32)
-       lo := int32(offset)
        // use GetFileType to check pipe, pipe can't do seek
        ft, _ := GetFileType(fd)
        if ft == FILE_TYPE_PIPE {
                return 0, ESPIPE
        }
+       if runtime.GOARCH == "amd64" {
+               err = setFilePointerEx(fd, offset, &newoffset, w)
+               return
+       }
+       hi := int32(offset >> 32)
+       lo := int32(offset)
        rlo, e := SetFilePointer(fd, lo, &hi, w)
        if e != nil {
                return 0, e

But that doesn't work. Looks like SetFilePointerEx comes with a bunch of warnings about multi-threaded programs and overlapped I/O. Not sure whether those apply.

Also, that GOARCH == "amd64" check is bogus. This should also work on 386 but I don't know how to shove a LARGE_INTEGER through syscall.Syscall6 on Windows.

Maybe @alexbrainman is interested in looking into this?

@ianlancetaylor

This comment has been minimized.

Show comment
Hide comment
@ianlancetaylor

ianlancetaylor Dec 6, 2017

Contributor

This is an existing issue. We can look at a CL, but otherwise kicking it down the road.

Contributor

ianlancetaylor commented Dec 6, 2017

This is an existing issue. We can look at a CL, but otherwise kicking it down the road.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.10, Go1.11 Dec 6, 2017

@heinrich5991

This comment has been minimized.

Show comment
Hide comment
@heinrich5991

heinrich5991 Dec 6, 2017

This is the how SetFilePointerEx is called in Rust, maybe you can copy that (except for the actual calling of the syscall): https://github.com/rust-lang/rust/blob/833785b090c30d4a359d901fb41bfafbe1607ce9/src/libstd/sys/windows/fs.rs#L337-L352.

I think the remarks about overlapped IO and multithreading are irrelevant for the implementation of this function, it's clear that two threads trying to set the file pointer and read a bit from the file must synchronize somehow, that's no difference between SetFilePointer and SetFilePointerEx.

heinrich5991 commented Dec 6, 2017

This is the how SetFilePointerEx is called in Rust, maybe you can copy that (except for the actual calling of the syscall): https://github.com/rust-lang/rust/blob/833785b090c30d4a359d901fb41bfafbe1607ce9/src/libstd/sys/windows/fs.rs#L337-L352.

I think the remarks about overlapped IO and multithreading are irrelevant for the implementation of this function, it's clear that two threads trying to set the file pointer and read a bit from the file must synchronize somehow, that's no difference between SetFilePointer and SetFilePointerEx.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Dec 7, 2017

Member

I think the remarks about overlapped IO and multithreading are irrelevant for the implementation

Yeah, true.

I just don't know the calling convention for GOARCH=386 passing a LARGE_INTEGER to Windows via Go's Syscall6.

But then again, my change also didn't work on amd64, so not sure what I screwed up.

Member

bradfitz commented Dec 7, 2017

I think the remarks about overlapped IO and multithreading are irrelevant for the implementation

Yeah, true.

I just don't know the calling convention for GOARCH=386 passing a LARGE_INTEGER to Windows via Go's Syscall6.

But then again, my change also didn't work on amd64, so not sure what I screwed up.

@alexbrainman

This comment has been minimized.

Show comment
Hide comment
@alexbrainman

alexbrainman Dec 7, 2017

Member

Maybe @alexbrainman is interested in looking into this?

I would like to fix this, but I have been busy lately. I will look into this when I have time, unless someone beats to me.

Alex

Member

alexbrainman commented Dec 7, 2017

Maybe @alexbrainman is interested in looking into this?

I would like to fix this, but I have been busy lately. I will look into this when I have time, unless someone beats to me.

Alex

@as

This comment has been minimized.

Show comment
Hide comment
@as

as Dec 7, 2017

Contributor

_, _, e1 := Syscall6(procSetFilePointerEx.Addr(), 4, uintptr(distToMove), uintptr(unsafe.Pointer(newFilePointer)), uintptr(whence), 0, 0, 0)

@bradfitz
The handle is passed in to setFilePointerEx, but it isn't given as an argument to the actuall syscall.Syscall6.

Contributor

as commented Dec 7, 2017

_, _, e1 := Syscall6(procSetFilePointerEx.Addr(), 4, uintptr(distToMove), uintptr(unsafe.Pointer(newFilePointer)), uintptr(whence), 0, 0, 0)

@bradfitz
The handle is passed in to setFilePointerEx, but it isn't given as an argument to the actuall syscall.Syscall6.

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Dec 7, 2017

Member

@as, hah. Whoops. Thanks!

Member

bradfitz commented Dec 7, 2017

@as, hah. Whoops. Thanks!

@gopherbot

This comment has been minimized.

Show comment
Hide comment
@gopherbot

gopherbot Dec 7, 2017

Change https://golang.org/cl/82535 mentions this issue: syscall: use SetFilePointerEx on Windows, allowing large seek offsets

gopherbot commented Dec 7, 2017

Change https://golang.org/cl/82535 mentions this issue: syscall: use SetFilePointerEx on Windows, allowing large seek offsets

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz
Member

bradfitz commented Dec 7, 2017

@bradfitz

This comment has been minimized.

Show comment
Hide comment
@bradfitz

bradfitz Dec 7, 2017

Member

And I can repro that my new test fails on 386 and passes with my new fix on amd64:

bradfitz@gdev:~/go/src/os$ gomote run user-bradfitz-windows-amd64-2016-0 go/bin/go test -short -v -run=Seek os
=== RUN   TestSeek
--- PASS: TestSeek (0.01s)
=== RUN   TestSeekError
--- PASS: TestSeekError (0.00s)
PASS
ok      os      0.045s
bradfitz@gdev:~/go/src/os$ gomote run -e GOARCH=386  user-bradfitz-windows-amd64-2016-0 go/bin/go test -short -v -run=Seek os
=== RUN   TestSeek
--- FAIL: TestSeek (0.01s)
        os_test.go:1382: #8: Seek(4294967295, 0) = 0, seek C:\Users\gopher\AppData\Local\Temp\1\_Go_TestSeek195906635: invalid argument want 4294967295, nil
        os_test.go:1382: #9: Seek(0, 1) = 0, seek C:\Users\gopher\AppData\Local\Temp\1\_Go_TestSeek195906635: invalid argument want 4294967295, nil
        os_test.go:1382: #10: Seek(8589934591, 0) = 0, seek C:\Users\gopher\AppData\Local\Temp\1\_Go_TestSeek195906635: invalid argument want 8589934591, nil
        os_test.go:1382: #11: Seek(0, 1) = 0, seek C:\Users\gopher\AppData\Local\Temp\1\_Go_TestSeek195906635: invalid argument want 8589934591, nil
=== RUN   TestSeekError
--- PASS: TestSeekError (0.00s)
FAIL
FAIL    os      0.137s
Error running run: exit status 1

Fixing on 386 now.

Member

bradfitz commented Dec 7, 2017

And I can repro that my new test fails on 386 and passes with my new fix on amd64:

bradfitz@gdev:~/go/src/os$ gomote run user-bradfitz-windows-amd64-2016-0 go/bin/go test -short -v -run=Seek os
=== RUN   TestSeek
--- PASS: TestSeek (0.01s)
=== RUN   TestSeekError
--- PASS: TestSeekError (0.00s)
PASS
ok      os      0.045s
bradfitz@gdev:~/go/src/os$ gomote run -e GOARCH=386  user-bradfitz-windows-amd64-2016-0 go/bin/go test -short -v -run=Seek os
=== RUN   TestSeek
--- FAIL: TestSeek (0.01s)
        os_test.go:1382: #8: Seek(4294967295, 0) = 0, seek C:\Users\gopher\AppData\Local\Temp\1\_Go_TestSeek195906635: invalid argument want 4294967295, nil
        os_test.go:1382: #9: Seek(0, 1) = 0, seek C:\Users\gopher\AppData\Local\Temp\1\_Go_TestSeek195906635: invalid argument want 4294967295, nil
        os_test.go:1382: #10: Seek(8589934591, 0) = 0, seek C:\Users\gopher\AppData\Local\Temp\1\_Go_TestSeek195906635: invalid argument want 8589934591, nil
        os_test.go:1382: #11: Seek(0, 1) = 0, seek C:\Users\gopher\AppData\Local\Temp\1\_Go_TestSeek195906635: invalid argument want 8589934591, nil
=== RUN   TestSeekError
--- PASS: TestSeekError (0.00s)
FAIL
FAIL    os      0.137s
Error running run: exit status 1

Fixing on 386 now.

@gopherbot gopherbot closed this in 7c46b62 Dec 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment