[bug] handle syscall errno in windows permissions #7059
Conversation
pkoutsovasilis
added
Team:Elastic-Agent-Control-Plane
| // Check for Errno = 0 which indicates success | ||
| // https://pkg.go.dev/golang.org/x/sys/windows#Errno | ||
| if errors.Is(err, syscall.Errno(0)) { | ||
| return nil |
There was a problem hiding this comment.
see here, previously if this was syscall.Errno(0), we were returning which IMO shouldn't be the case, and we should continue below trying to take onwership
|
buildkite test this |
pkoutsovasilis
force-pushed
the
fix/windows_permissions
branch
from
55990e1 to
44ac8e9
Compare
|
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
internal/pkg/agent/perms/windows.go
Outdated
| @@ -21,6 +21,15 @@ import ( | |||
| "github.com/elastic/elastic-agent/pkg/utils" | |||
| ) | |||
|
|
|||
| func errHandler(err error) error { | |||
There was a problem hiding this comment.
A clearer name would be better here. Being names errHandler doesn't really provide context to what it is doing, when reading the code where it is called from. Something like filterSuccessErr would be more appropriate.
| func errHandler(err error) error { | |
| func filterSuccessErr(err error) error { |
There was a problem hiding this comment.
Should we put this on every windows sycall we make? What is special about acl.Apply here?
There was a problem hiding this comment.
I believe that this error code (ERROR_SUCCESS) handling should happen for all functions that call directly the Win32 low level API. Now acl.Apply is actually calling this API here but doesn't check for it. Thus I don't believe we should add this check in every windows syscall 🙂
There was a problem hiding this comment.
thank you @blakerouse I was clearly lacking inspiration when naming this function but I do like your proposal! I just committed it 🙂
There was a problem hiding this comment.
@blakerouse @cmacknz should I consider this conversation as resolved? 🙂
|
💚 Build Succeeded
History
|
What does this PR do?
This PR attempts to fix an issue in Windows permission handling where
FixPermissionscould return an "Error" message stating "The operation completed successfully." The root cause most probably is the lack of explicit handling forsyscall.Errno(0), which indicates success but was mistakenly treated as an error. Also it fixes another issue where we weren't taking ownership of the files when we run into thesyscall.Errno(0)from the acl.ApplyWhy is it important?
Properly handling
syscall.Errno(0)prevents unnecessary error propagation and aligns the implementation with Windows system call behavior.Checklist
./changelog/fragmentsusing the changelog tool.Disruptive User Impact
This fix is not expected to cause any disruptions.
How to test this PR locally
N/A
Related issues