Address review comments on libuv patch #41
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
I'm testing Yggdrasil build here: JuliaPackaging/Yggdrasil#10316 |
staticfloat
added a commit
to JuliaLang/julia
that referenced
this pull request
Jan 22, 2025
This is to test JuliaLang/libuv#41
Member
Author
|
Julia testing PR: JuliaLang/julia#57126 |
staticfloat
added a commit
to JuliaLang/julia
that referenced
this pull request
Jan 23, 2025
This is to test JuliaLang/libuv#41
staticfloat
added a commit
to JuliaLang/julia
that referenced
this pull request
Feb 3, 2025
This is to test JuliaLang/libuv#41
staticfloat
added a commit
to JuliaLang/julia
that referenced
this pull request
Feb 4, 2025
This is to test JuliaLang/libuv#41
staticfloat
added a commit
to JuliaLang/julia
that referenced
this pull request
Feb 4, 2025
This is to test JuliaLang/libuv#41
staticfloat
force-pushed
the
sf/acl_rebase_new
branch
from
4eb819b to
772bf28
Compare
Member
Author
|
@vtjnash This passed tests in https://buildkite.com/julialang/julia-master/builds/44361#0194cf76-9df8-4206-bbb2-6bcbbade9ff3, so I've rebased to clean up history a bit and I think it's ready to merge now. |
Member
Author
|
One thing to note, the following tests needed to be adjusted: |
Member
|
CI did something weird there it seems. Can you rebase? I just pushed the various recent commits to fix CI from upstream |
staticfloat
force-pushed
the
sf/acl_rebase_new
branch
from
772bf28 to
12c6211
Compare
Member
|
I see; that wasn't actually your fault. I've fixed the branch content mistake, so now it should go through. |
No longer shy away from using `AccessCheck()` for simple `W_OK`/`R_OK` checks; use it for all calls unconditionally. It turns out that `F_OK` is supposed to only check for path existence, and because it is `0x00`, it ends up confusing the Windows APIs below where it looks like we're asking for the null set of permissions, so handle that as well.
Because our ACL implementation does not have the required flexibility to represent situations such as `0o757` due to the permissions ordering that Windows naturally applies, we simply do not allow the `other` entity to have permissions that the `group` entity does not have. This causes `chmod(0o757)` to be transparently mapped to `chmod(0o755)`, as an example.
Previously, we would apply permissions only to groups that we were a part of, but we should apply our "other" permissions to groups that we are not a part of.
So we must handle that appropriately in this helper function
* Use `DWORD` instead of `mode_t` on Windows. It's not needed to use `mode_t` here, and it just confuses VS.
* Define `S_IRWXU` and others if not already defined.
* When compiling on my windows testing box, I get the error:
```
error: initialization of 'SECURITY_INFORMATION' {aka 'long unsigned int'} from 'void *' makes integer from pointer without a cast [-Wint-conversion]
2317 | SECURITY_INFORMATION si = NULL;
```
staticfloat
force-pushed
the
sf/acl_rebase_new
branch
from
12c6211 to
46ca7fe
Compare
vtjnash
approved these changes
Feb 6, 2025
Keno
reviewed
Jul 17, 2025
|
|
||
| DWORD sdLen = 0, err = 0, tokenAccess = 0, desiredAccess = 0, | ||
| grantedAccess = 0, privilegesLen = 0; | ||
| SECURITY_INFORMATION si = (SECURITY_INFORMATION)NULL; |
Member
There was a problem hiding this comment.
SECURITY_INFORMATION is a DWORD, we shouldn't be converting from a ptr.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a new version of #16 based against
julia-uv2-1.48.0.This hopefully addresses the two actionable comments from
@erw7. In particular:We use
AccessCheck()unconditionally on duringaccess().I don't allow
otherto have more permissions thangroup. This is necessary because in cases likechmod("foo", 0o757), theDENYentry forgroup(necessary becauseotherhas more permissions thangroup) ends up blocking the user, due to the fact thatuserbelongs togroup. So I just mask out any bits thatotherhas thatgroupdoesn't, so0o757turns into0o755. You can still have0o057, or0o700or whatever, it's just the case whereotherhas bits thatgroupdoesn't that gets fixed up.