[go] os: do not call CreateSymbolicLink again if err is not ERROR_PRIVILEGE_NOT_HELD
Yasuhiro Matsumoto (Gerrit)
Yasuhiro Matsumoto has uploaded this change for review.
os: do not call CreateSymbolicLink again if err is not ERROR_PRIVILEGE_NOT_HELD
Updates #53582
Change-Id: I4495c5886410ed57d41625c088fbcf44e49bf1b9
---
M src/os/file_windows.go
1 file changed, 14 insertions(+), 0 deletions(-)
diff --git a/src/os/file_windows.go b/src/os/file_windows.go
index db5c27d..3cd4a0b 100644
--- a/src/os/file_windows.go
+++ b/src/os/file_windows.go
@@ -368,6 +368,9 @@
}
err = syscall.CreateSymbolicLink(n, o, flags)
if err != nil {
+ if err.(syscall.Errno) == syscall.ERROR_PRIVILEGE_NOT_HELD {
+ return &LinkError{"symlink", oldname, newname, err}
+ }
// the unprivileged create flag is unsupported
// below Windows 10 (1703, v10.0.14972). retry without it.
flags &^= windows.SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Yasuhiro Matsumoto (Gerrit)
Yasuhiro Matsumoto uploaded patch set #2 to this change.
os: do not call CreateSymbolicLink again if err is not ERROR_PRIVILEGE_NOT_HELD
Updates #53582
Change-Id: I4495c5886410ed57d41625c088fbcf44e49bf1b9
---
M src/os/file_windows.go
1 file changed, 14 insertions(+), 0 deletions(-)
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Yasuhiro Matsumoto (Gerrit)
1 comment:
Patchset:
I don't know how to reproduce/test #53582.
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Ian Lance Taylor, Yasuhiro Matsumoto.
Patch set 2:Run-TryBot +1Code-Review +1
2 comments:
Patchset:
TRY=windows-amd64-longtest
File src/os/file_windows.go:
if err != nil {
if err.(syscall.Errno) != syscall.ERROR_PRIVILEGE_NOT_HELD {
return &LinkError{"symlink", oldname, newname, err}
}
This seems like it would be a bit simpler as two sequential conditions instead of one nested one:
```
if err == syscall.ERROR_PRIVILEGE_NOT_HELD {
// the unprivileged create flag is unsupported
// below Windows 10 (1703, v10.0.14972). retry without it.
flags &^= windows.SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE
err = syscall.CreateSymbolicLink(n, o, flags)
}
if err != nil {
return &LinkError{"symlink", oldname, newname, err}
}
return nil
```
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Yasuhiro Matsumoto (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Ian Lance Taylor, Yasuhiro Matsumoto.
Yasuhiro Matsumoto uploaded patch set #3 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Bryan Mills, TryBot-Result-1 by Gopher Robot
os: do not call CreateSymbolicLink again if err is not ERROR_PRIVILEGE_NOT_HELD nor ERROR_INVALID_PARAMETER
Updates #53582
Change-Id: I4495c5886410ed57d41625c088fbcf44e49bf1b9
---
M src/os/file_windows.go
1 file changed, 15 insertions(+), 0 deletions(-)
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Yasuhiro Matsumoto (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Ian Lance Taylor, Yasuhiro Matsumoto.
Yasuhiro Matsumoto uploaded patch set #4 to this change.
os: do not call CreateSymbolicLink again if err is not ERROR_PRIVILEGE_NOT_HELD nor ERROR_INVALID_PARAMETER
Updates #53582
Change-Id: I4495c5886410ed57d41625c088fbcf44e49bf1b9
---
M src/os/file_windows.go
1 file changed, 15 insertions(+), 0 deletions(-)
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Yasuhiro Matsumoto (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Ian Lance Taylor.
2 comments:
Patchset:
Thank you for review.
File src/os/file_windows.go:
if err != nil {
if err.(syscall.Errno) != syscall.ERROR_PRIVILEGE_NOT_HELD {
return &LinkError{"symlink", oldname, newname, err}
}
This seems like it would be a bit simpler as two sequential conditions instead of one nested one: […]
Done
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Ian Lance Taylor (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Yasuhiro Matsumoto.
Patch set 4:Run-TryBot +1
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Yasuhiro Matsumoto.
Patch set 4:Code-Review +1
2 comments:
File src/os/file_windows.go:
if err != nil {
if err.(syscall.Errno) != syscall.ERROR_PRIVILEGE_NOT_HELD {
return &LinkError{"symlink", oldname, newname, err}
}
Done
Does not appear to be done?
I would ideally like to see only one `return &LinkError{ … }` for the case where the `CreateSymbolicLink` call fails:
```
err = syscall.CreateSymbolicLink(n, o, flags)
if err == windows.ERROR_INVALID_PARAMETER {
// the unprivileged create flag is unsupported
// below Windows 10 (1703, v10.0.14972). retry without it.
flags &^= windows.SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE
err = syscall.CreateSymbolicLink(n, o, flags)
}
if err != nil {
return &LinkError{"symlink", oldname, newname, err}
}
return nil
```
File src/os/file_windows.go:
Patch Set #4, Line 371:
ERROR_PRIVILEGE_NOT_HELDHmm... is the `ERROR_PRIVILEGE_NOT_HELD` error actually relevant here at all?
This is a retry without the `SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE` flag, but if the privilege is actually required then retrying _without_ that flag won't fix that problem anyway.
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Yasuhiro Matsumoto (Gerrit)
Attention is currently required from: Alex Brainman, Ian Lance Taylor, Yasuhiro Matsumoto.
Yasuhiro Matsumoto uploaded patch set #5 to this change.
The following approvals got outdated and were removed: Run-TryBot+1 by Ian Lance Taylor, TryBot-Result+1 by Gopher Robot
os: do not call CreateSymbolicLink again if err is not ERROR_PRIVILEGE_NOT_HELD nor ERROR_INVALID_PARAMETER
Updates #53582
Change-Id: I4495c5886410ed57d41625c088fbcf44e49bf1b9
---
M src/os/file_windows.go
1 file changed, 15 insertions(+), 4 deletions(-)
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Yasuhiro Matsumoto (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Ian Lance Taylor.
2 comments:
File src/os/file_windows.go:
if err != nil {
if err.(syscall.Errno) != syscall.ERROR_PRIVILEGE_NOT_HELD {
return &LinkError{"symlink", oldname, newname, err}
}
Does not appear to be done? […]
Done
File src/os/file_windows.go:
Patch Set #4, Line 371:
ERROR_PRIVILEGE_NOT_HELD
Hmm... is the `ERROR_PRIVILEGE_NOT_HELD` error actually relevant here at all? […]
When the user does not have privileges, error "A required privilege is not held by the client." is returned.
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Ian Lance Taylor, Yasuhiro Matsumoto.
Patch set 5:Code-Review +1
1 comment:
File src/os/file_windows.go:
Patch Set #4, Line 371:
ERROR_PRIVILEGE_NOT_HELD
When the user does not have privileges, error "A required privilege is not held by the client." is returned.
Right, but when the user does not have privileges, retrying _without_ the `SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE` flag is not going to do anything to fix that situation. (We should skip the overhead of the second call to retry.)
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Yasuhiro Matsumoto (Gerrit)
Attention is currently required from: Alex Brainman, Bryan Mills, Ian Lance Taylor.
1 comment:
File src/os/file_windows.go:
Patch Set #4, Line 371:
ERROR_PRIVILEGE_NOT_HELD
> When the user does not have privileges, error "A required privilege is not held by the client. […]
Sorry, I'm confusing. first call possibly return two errors.
1. ERROR_PRIVILEGE_NOT_HELD
This is failure that the user does not have privileges to create symbolic link. So we should call again without SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag.2. ERROR_INVALID_PARAMETER
This is failure that the Windows version does not support SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag. So try again without SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE.Right?
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Bryan Mills (Gerrit)
Attention is currently required from: Alex Brainman, Ian Lance Taylor, Yasuhiro Matsumoto.
1 comment:
File src/os/file_windows.go:
Patch Set #4, Line 371:
ERROR_PRIVILEGE_NOT_HELD
Sorry, I'm confusing. first call possibly return two errors.
1. ERROR_PRIVILEGE_NOT_HELD
This is failure that the user does not have privileges to create symbolic link. So we should call again without SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag.
From https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createsymboliclinkw, my understanding is:
- If the system is in developer mode, `SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE` allows users to create symlinks (regardless of their privilege level).
- If the system is not in developer mode, then only privileged users can create symlinks (regardless of `SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE`).
So setting `SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE` can only allow `CreateSymbolicLink` to succeed when it would otherwise fail — the flag cannot cause it to fail when it would otherwise succeed.
If the first call fails with `ERROR_PRIVILEGE_NOT_HELD`, that means that the user is unprivileged *and* the system is not in developer mode. In that case, retrying without the flag still will not succeed (because the user is still unprivileged.)
> 2. ERROR_INVALID_PARAMETER
> This is failure that the Windows version does not support SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE flag. So try again without SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE.
Correct.
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Attention is currently required from: Bryan Mills, Ian Lance Taylor, Yasuhiro Matsumoto.
2 comments:
Patchset:
Thanks for trying to fix this Matn,
Please see my comment.
Alex
File src/os/file_windows.go:
Patch Set #4, Line 371:
ERROR_PRIVILEGE_NOT_HELD
> Sorry, I'm confusing. first call possibly return two errors. […]
Matn, I don't see purpose of this change. Why do you think it will help with issue #53582? And how do you know your change will not break any existing users?
I am worried, because we don't have many tests for link creation.
To view, visit change 415094. To unsubscribe, or for help writing mail filters, visit settings.