[go] os,syscall: File.Stat to use file handle for directories on Windows
Quim Muntal (Gerrit)
Attention is currently required from: Roland Shoemaker.
Quim Muntal uploaded patch set #4 to this change.
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories.
This allows to use directory handles in GetFileInformationByHandle,
which is used by File.Stat on Windows, and avoids relying on
path-based windows APIs for this operation.
File.readdir is left using FindFirstFile and friends, but now that
we have a proper directory handle, we can refactor readdir to use
NtQueryDirectoryFile in the future CL.
Updates #52747
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
6 files changed, 84 insertions(+), 51 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Roland Shoemaker.
Quim Muntal uploaded patch set #5 to this change.
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories.
This allows to use directory handles in GetFileInformationByHandle,
which is used by File.Stat on Windows, and avoids relying on
path-based windows APIs for this operation.
File.readdir is left using FindFirstFile and friends, but now that
we have a proper directory handle, we can refactor readdir to use
NtQueryDirectoryFile in the future CL.
Fixes #52747
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
6 files changed, 84 insertions(+), 51 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Roland Shoemaker.
Quim Muntal uploaded patch set #6 to this change.
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories.
This allows to use directory handles in GetFileInformationByHandle,
which is used by File.Stat on Windows, and avoids relying on
path-based windows APIs for this operation.
File.readdir is left using FindFirstFile and friends, but now that
we have a proper directory handle, we can refactor readdir to use
NtQueryDirectoryFile in the future CL.
Fixes #52747
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
6 files changed, 85 insertions(+), 51 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Roland Shoemaker.
Quim Muntal uploaded patch set #7 to this change.
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories.
This allows to use directory handles in GetFileInformationByHandle,
which is used by File.Stat on Windows, and avoids relying on
path-based windows APIs for this operation.
File.readdir is left using FindFirstFile and friends, but now that
we have a proper directory handle, we can refactor readdir to use
NtQueryDirectoryFile in the future CL.
Fixes #52747
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
6 files changed, 85 insertions(+), 60 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Roland Shoemaker.
Quim Muntal uploaded patch set #8 to this change.
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories.
This allows to use directory handles in GetFileInformationByHandle,
which is used by File.Stat on Windows, and avoids relying on
path-based windows APIs for this operation.
File.readdir is left using FindFirstFile and friends, but now that
we have a proper directory handle, we can refactor readdir to use
NtQueryDirectoryFile in the future CL.
Fixes #52747
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file_windows.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
6 files changed, 94 insertions(+), 67 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Quim Muntal, Roland Shoemaker.
Patch set 8:Run-TryBot +1
1 comment:
Patchset:
Thank you for working on this.
I think you should add new test that checks that "TOCTOU issue" from https://github.com/golang/go/issues/52747#issue-1228127397 is gone.
Alex
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Quim Muntal, Roland Shoemaker.
Quim Muntal uploaded patch set #9 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Alex Brainman, TryBot-Result-1 by Gopher Robot
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories.
This allows to use directory handles in GetFileInformationByHandle,
which is used by File.Stat on Windows, and avoids relying on
path-based windows APIs for this operation.
File.readdir is left using FindFirstFile and friends, but now that
we have a proper directory handle, we can refactor readdir to use
NtQueryDirectoryFile in the future CL.
Fixes #52747
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file_windows.go
M src/os/os_test.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
7 files changed, 136 insertions(+), 67 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Roland Shoemaker.
2 comments:
Patchset:
Thank you for working on this. […]
I can't add a test that checks for TOCTOU issues in file.Stat(), as this CL makes the directory handle to be opened without `FILE_SHARE_DELETE` shared access, therefore can't rename the directory to reproduce a TOCTOU until the handle is closed.
I did add a test to validate os.Open() opens directories without `FILE_SHARE_DELETE` shared access.
3 of 30 TryBots failed. […]
Done
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Patrik Nyblom (Gerrit)
Attention is currently required from: Alex Brainman, Quim Muntal, Roland Shoemaker.
Patch set 9:Run-TryBot +1Code-Review +1
1 comment:
File src/syscall/syscall_windows.go:
Patch Set #9, Line 375:
attrs |= FILE_FLAG_BACKUP_SEMANTICSThis would happen for ordinary files as well, right? Is there really no change in semantics for an ordinary file to be opened with this flag? Is there something that should be put in the release notes?
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Patrik Nyblom (Gerrit)
Attention is currently required from: Alex Brainman, Quim Muntal, Roland Shoemaker.
Patch set 9:-Code-Review
2 comments:
Patchset:
Thanks for working on this. Looks good!
Unfortunately this seems to break trybots as you're referring to windows files in the os_test.
File src/os/os_test.go:
Patch Set #9, Line 12:
"internal/syscall/windows"I don't think you can do this here, you would have to do this in a windows specific test.
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Quim Muntal, Roland Shoemaker.
Quim Muntal uploaded patch set #10 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Patrik Nyblom, TryBot-Result-1 by Gopher Robot
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories.
This allows to use directory handles in GetFileInformationByHandle,
which is used by File.Stat on Windows, and avoids relying on
path-based windows APIs for this operation.
File.readdir is left using FindFirstFile and friends, but now that
we have a proper directory handle, we can refactor readdir to use
NtQueryDirectoryFile in the future CL.
Fixes #52747
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
7 files changed, 132 insertions(+), 67 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Roland Shoemaker.
1 comment:
Patchset:
Thanks for working on this. Looks good! […]
Wops, my bad. I've moved the offending test to os_windows_test.go
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Roland Shoemaker.
Patch set 10:Run-TryBot +1
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Patrik Nyblom, Roland Shoemaker.
2 comments:
File src/os/os_test.go:
Patch Set #9, Line 12:
"internal/syscall/windows"I don't think you can do this here, you would have to do this in a windows specific test.
Done
File src/syscall/syscall_windows.go:
Patch Set #9, Line 375:
attrs |= FILE_FLAG_BACKUP_SEMANTICS
This would happen for ordinary files as well, right? Is there really no change in semantics for an o […]
There are probably some things to put in the release notes due to this CL, I've posted this comment to discuss the implications: https://github.com/golang/go/issues/52747#issuecomment-1272950285
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Patrik Nyblom (Gerrit)
Attention is currently required from: Alex Brainman, Quim Muntal, Roland Shoemaker.
Patch set 10:Code-Review +1
2 comments:
Patchset:
Thanks for this! The code LGTM. If Alex, Russ and Ian are fine with the behavior changes described in https://github.com/golang/go/issues/52747#issuecomment-1272950285, I have no objections.
File src/os/os_test.go:
Patch Set #9, Line 12:
"internal/syscall/windows"
Done
Thanks!
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Quim Muntal, Roland Shoemaker.
Patch set 10:Code-Review +1
4 comments:
Patchset:
LGTM.
But leaving for others to decide.
Thank you.
Alex
File src/os/os_windows_test.go:
Patch Set #10, Line 1262:
Errors/Error/Fatal/
and then you will not need return on line 1263.
Just like you did on line 1258.
File src/syscall/syscall_windows_test.go:
Patch Set #10, Line 28:
t.Error("Open should have failed")Should you call
syscall.CloseHandle(h)
here? To allow for test code delete tmp directories.
Patch Set #10, Line 32:
t.Error("Open should have failed")Same.
Should you call
syscall.CloseHandle(h)
here? To allow for test code delete tmp directories.
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Quim Muntal, Roland Shoemaker.
Quim Muntal uploaded patch set #11 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Quim Muntal, TryBot-Result+1 by Gopher Robot
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories.
This allows to use directory handles in GetFileInformationByHandle,
which is used by File.Stat on Windows, and avoids relying on
path-based windows APIs for this operation.
File.readdir is left using FindFirstFile and friends, but now that
we have a proper directory handle, we can refactor readdir to use
NtQueryDirectoryFile in the future CL.
Fixes #52747
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file_windows.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
7 files changed, 135 insertions(+), 67 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Roland Shoemaker.
Patch set 11:Run-TryBot +1
3 comments:
File src/os/os_windows_test.go:
s/Error/Fatal/ […]
Done
File src/syscall/syscall_windows_test.go:
Patch Set #10, Line 28:
t.Error("Open should have failed")
Should you call […]
Done
Patch Set #10, Line 32:
t.Error("Open should have failed")
Same. […]
Done
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Quim Muntal, Roland Shoemaker.
Bryan Mills removed a vote from this change.
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Quim Muntal, Roland Shoemaker.
Bryan Mills removed a vote from this change.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Quim Muntal, Roland Shoemaker.
Patch set 11:Run-TryBot +1
1 comment:
Patchset:
TRY=windows-amd64-longtest,windows-arm64-11,x/tools@windows-amd64-longtest,x/tools@windows-arm64-11
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Quim Muntal, Roland Shoemaker.
Quim Muntal uploaded patch set #12 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Bryan Mills, TryBot-Result+1 by Gopher Robot
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories via CreateFileW.
CreateFileW handles are more versatile than FindFirstFile handles.
They can be used in Win32 APIs like GetFileInformationByHandle and
SetFilePointerEx, which are needed by some Go APIs.
Fixes #52747
Fixes #36019
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file.go
M src/os/file_windows.go
M src/os/os_test.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
9 files changed, 133 insertions(+), 74 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Roland Shoemaker.
Patch set 12:Run-TryBot +1
1 comment:
Patchset:
I've updated this CL to prove that it fixes #36019.
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Quim Muntal, Roland Shoemaker.
Roland Shoemaker removed a vote from this change.
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Quim Muntal, Roland Shoemaker.
Roland Shoemaker removed a vote from this change.
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Quim Muntal.
Patch set 12:Run-TryBot +1
2 comments:
Patchset:
Looks good from a security perspective, as I understand it this closes the TOCTOU issues we previously found. Leaving it up to others to +2.
1 of 34 SlowBots failed. […]
Looks like a known flake.
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Quim Muntal.
Patch set 12:Code-Review +1
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Quim Muntal.
Patch set 12:Code-Review +2
1 comment:
File src/os/file_windows.go:
Patch Set #11, Line 89:
h syscall.HandlePlease add a comment (or clarify the field name) to indicate that this is a search handle (as opposed to, say, a file or directory handle).
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Quim Muntal.
1 comment:
Patchset:
RELNOTE=yes
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Quim Muntal.
Patch set 12:Code-Review +1
1 comment:
Patchset:
Still LGTM.
Thank you.
Alex
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Quim Muntal, Roland Shoemaker.
Quim Muntal uploaded patch set #13 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Roland Shoemaker, TryBot-Result+1 by Gopher Robot
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories via CreateFileW.
CreateFileW handles are more versatile than FindFirstFile handles.
They can be used in Win32 APIs like GetFileInformationByHandle and
SetFilePointerEx, which are needed by some Go APIs.
Fixes #52747
Fixes #36019
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file.go
M src/os/file_windows.go
M src/os/os_test.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
9 files changed, 133 insertions(+), 74 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Roland Shoemaker.
Patch set 13:Run-TryBot +1
1 comment:
File src/os/file_windows.go:
Patch Set #11, Line 89:
h syscall.Handle
Please add a comment (or clarify the field name) to indicate that this is a search handle (as oppose […]
Done
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Quim Muntal, Roland Shoemaker.
Quim Muntal uploaded patch set #14 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Quim Muntal, TryBot-Result+1 by Gopher Robot
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories via CreateFileW.
CreateFileW handles are more versatile than FindFirstFile handles.
They can be used in Win32 APIs like GetFileInformationByHandle and
SetFilePointerEx, which are needed by some Go APIs.
Fixes #52747
Fixes #36019
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file.go
M src/os/file_windows.go
M src/os/os_test.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
9 files changed, 133 insertions(+), 74 deletions(-)
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
Attention is currently required from: Roland Shoemaker.
Patch set 14:Run-TryBot +1
Quim Muntal (Gerrit)
Attention is currently required from: Patrik Nyblom, Roland Shoemaker.
1 comment:
File src/syscall/syscall_windows.go:
Patch Set #9, Line 375:
attrs |= FILE_FLAG_BACKUP_SEMANTICS
There are probably some things to put in the release notes due to this CL, I've posted this comment […]
There has been some discussion in that issue about the semantic changes, and there seems to be an agreement that this change makes sense. Marking as resolved.
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Roland Shoemaker (Gerrit)
Roland Shoemaker submitted this change.
12 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: src/os/file_windows.go
Insertions: 1, Deletions: 1.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/os/stat_windows.go
Insertions: 1, Deletions: 20.
The diff is too large to show. Please review the diff.
```
```
The name of the file: src/os/os_windows_test.go
Insertions: 0, Deletions: 16.
The diff is too large to show. Please review the diff.
```
os,syscall: File.Stat to use file handle for directories on Windows
Updates syscall.Open to support opening directories via CreateFileW.
CreateFileW handles are more versatile than FindFirstFile handles.
They can be used in Win32 APIs like GetFileInformationByHandle and
SetFilePointerEx, which are needed by some Go APIs.
Fixes #52747
Fixes #36019
Change-Id: I26a00cef9844fb4abeeb18d2f9d854162a146651
Reviewed-on: https://go-review.googlesource.com/c/go/+/405275
Reviewed-by: Roland Shoemaker <rol...@golang.org>
Reviewed-by: Patrik Nyblom <pn...@google.com>
Reviewed-by: Alex Brainman <alex.b...@gmail.com>
Reviewed-by: Bryan Mills <bcm...@google.com>
Run-TryBot: Quim Muntal <quimm...@gmail.com>
TryBot-Result: Gopher Robot <go...@golang.org>
---
M src/internal/poll/fd_windows.go
M src/os/dir_windows.go
M src/os/file.go
M src/os/file_windows.go
M src/os/os_test.go
M src/os/os_windows_test.go
M src/os/stat_windows.go
M src/syscall/syscall_windows.go
M src/syscall/syscall_windows_test.go
9 files changed, 140 insertions(+), 74 deletions(-)
diff --git a/src/internal/poll/fd_windows.go b/src/internal/poll/fd_windows.go
index 1af2011..3a4a74f 100644
--- a/src/internal/poll/fd_windows.go
+++ b/src/internal/poll/fd_windows.go
@@ -268,7 +268,6 @@
kindNet fileKind = iota
kindFile
kindConsole
- kindDir
kindPipe
)
@@ -286,12 +285,10 @@
}
switch net {
- case "file":
+ case "file", "dir":
fd.kind = kindFile
case "console":
fd.kind = kindConsole
- case "dir":
- fd.kind = kindDir
case "pipe":
fd.kind = kindPipe
case "tcp", "tcp4", "tcp6",
@@ -371,8 +368,6 @@
case kindNet:
// The net package uses the CloseFunc variable for testing.
err = CloseFunc(fd.Sysfd)
- case kindDir:
- err = syscall.FindClose(fd.Sysfd)
default:
err = syscall.CloseHandle(fd.Sysfd)
}
@@ -1008,15 +1003,6 @@
return syscall.Seek(fd.Sysfd, offset, whence)
}
-// FindNextFile wraps syscall.FindNextFile.
-func (fd *FD) FindNextFile(data *syscall.Win32finddata) error {
- if err := fd.incref(); err != nil {
- return err
- }
- defer fd.decref()
- return syscall.FindNextFile(fd.Sysfd, data)
-}
-
// Fchmod updates syscall.ByHandleFileInformation.Fileattributes when needed.
func (fd *FD) Fchmod(mode uint32) error {
if err := fd.incref(); err != nil {
diff --git a/src/os/dir_windows.go b/src/os/dir_windows.go
index 253adad..445e4f7 100644
--- a/src/os/dir_windows.go
+++ b/src/os/dir_windows.go
@@ -11,8 +11,15 @@
)
func (file *File) readdir(n int, mode readdirMode) (names []string, dirents []DirEntry, infos []FileInfo, err error) {
- if !file.isdir() {
- return nil, nil, nil, &PathError{Op: "readdir", Path: file.name, Err: syscall.ENOTDIR}
+ // If this file has no dirinfo, create one.
+ needdata := true
+ if file.dirinfo == nil {
+ needdata = false
+ file.dirinfo, err = openDir(file.name)
+ if err != nil {
+ err = &PathError{Op: "readdir", Path: file.name, Err: err}
+ return
+ }
}
wantAll := n <= 0
if wantAll {
@@ -20,8 +27,8 @@
}
d := &file.dirinfo.data
for n != 0 && !file.dirinfo.isempty {
- if file.dirinfo.needdata {
- e := file.pfd.FindNextFile(d)
+ if needdata {
+ e := syscall.FindNextFile(file.dirinfo.h, d)
runtime.KeepAlive(file)
if e != nil {
if e == syscall.ERROR_NO_MORE_FILES {
@@ -32,7 +39,7 @@
}
}
}
- file.dirinfo.needdata = true
+ needdata = true
name := syscall.UTF16ToString(d.FileName[0:])
if name == "." || name == ".." { // Useless names
continue
diff --git a/src/os/file.go b/src/os/file.go
index 070ccd0..753aeb6 100644
--- a/src/os/file.go
+++ b/src/os/file.go
@@ -225,10 +225,6 @@
// relative to the current offset, and 2 means relative to the end.
// It returns the new offset and an error, if any.
// The behavior of Seek on a file opened with O_APPEND is not specified.
-//
-// If f is a directory, the behavior of Seek varies by operating
-// system; you can seek to the beginning of the directory on Unix-like
-// operating systems, but not on Windows.
func (f *File) Seek(offset int64, whence int) (ret int64, err error) {
if err := f.checkValid("seek"); err != nil {
return 0, err
diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index db5c27d..d94b78f 100644
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -86,10 +86,14 @@
// Auxiliary information if the File describes a directory
type dirInfo struct {
- data syscall.Win32finddata
- needdata bool
- path string
- isempty bool // set if FindFirstFile returns ERROR_FILE_NOT_FOUND
+ h syscall.Handle // search handle created with FindFirstFile
+ data syscall.Win32finddata
+ path string
+ isempty bool // set if FindFirstFile returns ERROR_FILE_NOT_FOUND
+}
+
+func (d *dirInfo) close() error {
+ return syscall.FindClose(d.h)
}
func epipecheck(file *File, e error) {
@@ -99,17 +103,7 @@
// On Unix-like systems, it is "/dev/null"; on Windows, "NUL".
const DevNull = "NUL"
-func (f *file) isdir() bool { return f != nil && f.dirinfo != nil }
-
-func openFile(name string, flag int, perm FileMode) (file *File, err error) {
- r, e := syscall.Open(fixLongPath(name), flag|syscall.O_CLOEXEC, syscallMode(perm))
- if e != nil {
- return nil, e
- }
- return newFile(r, name, "file"), nil
-}
-
-func openDir(name string) (file *File, err error) {
+func openDir(name string) (d *dirInfo, e error) {
var mask string
path := fixLongPath(name)
@@ -130,25 +124,27 @@
if e != nil {
return nil, e
}
- d := new(dirInfo)
- r, e := syscall.FindFirstFile(maskp, &d.data)
+ d = new(dirInfo)
+ d.h, e = syscall.FindFirstFile(maskp, &d.data)
if e != nil {
// FindFirstFile returns ERROR_FILE_NOT_FOUND when
// no matching files can be found. Then, if directory
// exists, we should proceed.
- if e != syscall.ERROR_FILE_NOT_FOUND {
- return nil, e
- }
+ // If FindFirstFile failed because name does not point
+ // to a directory, we should return ENOTDIR.
var fa syscall.Win32FileAttributeData
- pathp, e := syscall.UTF16PtrFromString(path)
- if e != nil {
+ pathp, e1 := syscall.UTF16PtrFromString(path)
+ if e1 != nil {
return nil, e
}
- e = syscall.GetFileAttributesEx(pathp, syscall.GetFileExInfoStandard, (*byte)(unsafe.Pointer(&fa)))
- if e != nil {
+ e1 = syscall.GetFileAttributesEx(pathp, syscall.GetFileExInfoStandard, (*byte)(unsafe.Pointer(&fa)))
+ if e1 != nil {
return nil, e
}
if fa.FileAttributes&syscall.FILE_ATTRIBUTE_DIRECTORY == 0 {
+ return nil, syscall.ENOTDIR
+ }
+ if e != syscall.ERROR_FILE_NOT_FOUND {
return nil, e
}
d.isempty = true
@@ -157,12 +153,11 @@
if !isAbs(d.path) {
d.path, e = syscall.FullPath(d.path)
if e != nil {
+ d.close()
return nil, e
}
}
- f := newFile(r, name, "dir")
- f.dirinfo = d
- return f, nil
+ return d, nil
}
// openFileNolog is the Windows implementation of OpenFile.
@@ -170,28 +165,36 @@
if name == "" {
return nil, &PathError{Op: "open", Path: name, Err: syscall.ENOENT}
}
- r, errf := openFile(name, flag, perm)
- if errf == nil {
- return r, nil
- }
- r, errd := openDir(name)
- if errd == nil {
- if flag&O_WRONLY != 0 || flag&O_RDWR != 0 {
- r.Close()
- return nil, &PathError{Op: "open", Path: name, Err: syscall.EISDIR}
+ path := fixLongPath(name)
+ r, e := syscall.Open(path, flag|syscall.O_CLOEXEC, syscallMode(perm))
+ if e != nil {
+ // We should return EISDIR when we are trying to open a directory with write access.
+ if e == syscall.ERROR_ACCESS_DENIED && (flag&O_WRONLY != 0 || flag&O_RDWR != 0) {
+ pathp, e1 := syscall.UTF16PtrFromString(path)
+ if e1 == nil {
+ var fa syscall.Win32FileAttributeData
+ e1 = syscall.GetFileAttributesEx(pathp, syscall.GetFileExInfoStandard, (*byte)(unsafe.Pointer(&fa)))
+ if e1 == nil && fa.FileAttributes&syscall.FILE_ATTRIBUTE_DIRECTORY != 0 {
+ e = syscall.EISDIR
+ }
+ }
}
- return r, nil
+ return nil, &PathError{Op: "open", Path: name, Err: e}
}
- return nil, &PathError{Op: "open", Path: name, Err: errf}
+ f, e := newFile(r, name, "file"), nil
+ if e != nil {
+ return nil, &PathError{Op: "open", Path: name, Err: e}
+ }
+ return f, nil
}
func (file *file) close() error {
if file == nil {
return syscall.EINVAL
}
- if file.isdir() && file.dirinfo.isempty {
- // "special" empty directories
- return nil
+ if file.dirinfo != nil {
+ file.dirinfo.close()
+ file.dirinfo = nil
}
var err error
if e := file.pfd.Close(); e != nil {
@@ -211,6 +214,12 @@
// relative to the current offset, and 2 means relative to the end.
// It returns the new offset and an error, if any.
func (f *File) seek(offset int64, whence int) (ret int64, err error) {
+ if f.dirinfo != nil {
+ // Free cached dirinfo, so we allocate a new one if we
+ // access this file as a directory again. See #35767 and #37161.
+ f.dirinfo.close()
+ f.dirinfo = nil
+ }
ret, err = f.pfd.Seek(offset, whence)
runtime.KeepAlive(f)
return ret, err
diff --git a/src/os/os_test.go b/src/os/os_test.go
index ee030a8..e548777 100644
--- a/src/os/os_test.go
+++ b/src/os/os_test.go
@@ -2564,9 +2564,6 @@
}
func TestDirSeek(t *testing.T) {
- if runtime.GOOS == "windows" {
- testenv.SkipFlaky(t, 36019)
- }
wd, err := Getwd()
if err != nil {
t.Fatal(err)
diff --git a/src/os/os_windows_test.go b/src/os/os_windows_test.go
index 55253cd..b9fad71 100644
--- a/src/os/os_windows_test.go
+++ b/src/os/os_windows_test.go
@@ -1252,3 +1252,28 @@
mklink(t, "relfilelink", "file")
testReadlink(t, "relfilelink", "file")
}
+
+func TestOpenDirTOCTOU(t *testing.T) {
+ // Check opened directories can't be renamed until the handle is closed.
+ // See issue 52747.
+ tmpdir := t.TempDir()
+ dir := filepath.Join(tmpdir, "dir")
+ if err := os.Mkdir(dir, 0777); err != nil {
+ t.Fatal(err)
+ }
+ f, err := os.Open(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ newpath := filepath.Join(tmpdir, "dir1")
+ err = os.Rename(dir, newpath)
+ if err == nil || !errors.Is(err, windows.ERROR_SHARING_VIOLATION) {
+ f.Close()
+ t.Fatalf("Rename(%q, %q) = %v; want windows.ERROR_SHARING_VIOLATION", dir, newpath, err)
+ }
+ f.Close()
+ err = os.Rename(dir, newpath)
+ if err != nil {
+ t.Error(err)
+ }
+}
diff --git a/src/os/stat_windows.go b/src/os/stat_windows.go
index f8f229c..8747c19 100644
--- a/src/os/stat_windows.go
+++ b/src/os/stat_windows.go
@@ -16,10 +16,6 @@
if file == nil {
return nil, ErrInvalid
}
- if file.isdir() {
- // I don't know any better way to do that for directory
- return Stat(file.dirinfo.path)
- }
return statHandle(file.name, file.pfd.Sysfd)
}
diff --git a/src/syscall/syscall_windows.go b/src/syscall/syscall_windows.go
index 9547ae0..4fbcdcd 100644
--- a/src/syscall/syscall_windows.go
+++ b/src/syscall/syscall_windows.go
@@ -371,8 +371,11 @@
}
}
}
- h, e := CreateFile(pathp, access, sharemode, sa, createmode, attrs, 0)
- return h, e
+ if createmode == OPEN_EXISTING && access == GENERIC_READ {
+ // Necessary for opening directory handles.
+ attrs |= FILE_FLAG_BACKUP_SEMANTICS
+ }
+ return CreateFile(pathp, access, sharemode, sa, createmode, attrs, 0)
}
func Read(fd Handle, p []byte) (n int, err error) {
diff --git a/src/syscall/syscall_windows_test.go b/src/syscall/syscall_windows_test.go
index 87f6580b..3b56721 100644
--- a/src/syscall/syscall_windows_test.go
+++ b/src/syscall/syscall_windows_test.go
@@ -15,6 +15,28 @@
"testing"
)
+func TestOpen_Dir(t *testing.T) {
+ dir := t.TempDir()
+
+ h, err := syscall.Open(dir, syscall.O_RDONLY, 0)
+ if err != nil {
+ t.Fatalf("Open failed: %v", err)
+ }
+ syscall.CloseHandle(h)
+ h, err = syscall.Open(dir, syscall.O_RDONLY|syscall.O_TRUNC, 0)
+ if err == nil {
+ t.Error("Open should have failed")
+ } else {
+ syscall.CloseHandle(h)
+ }
+ h, err = syscall.Open(dir, syscall.O_RDONLY|syscall.O_CREAT, 0)
+ if err == nil {
+ t.Error("Open should have failed")
+ } else {
+ syscall.CloseHandle(h)
+ }
+}
+
func TestWin32finddata(t *testing.T) {
dir := t.TempDir()
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Quim Muntal.
1 comment:
File src/os/os_test.go:
// See issue 37161. Read only one entry from a directory,
// seek to the beginning, and read again. We should not see
// duplicate entries.
if runtime.GOOS == "windows" {
testenv.SkipFlaky(t, 36019)
}Should this skip have been removed too?
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.
Quim Muntal (Gerrit)
1 comment:
File src/os/os_test.go:
// See issue 37161. Read only one entry from a directory,
// seek to the beginning, and read again. We should not see
// duplicate entries.
if runtime.GOOS == "windows" {
testenv.SkipFlaky(t, 36019)
}Should this skip have been removed too?
Good catch. Submitted https://go-review.googlesource.com/c/go/+/458336 to reenable it.
To view, visit change 405275. To unsubscribe, or for help writing mail filters, visit settings.