Address review comments on libuv patch #16

staticfloat · 2020-10-15T01:45:39Z

This hopefully addresses the two actionable comments from @erw7. In particular:

We use AccessCheck() unconditionally on during access().
I don't allow other to have more permissions than group. This is necessary because in cases like chmod("foo", 0o757), the DENY entry for group (necessary because other has more permissions than group) ends up blocking the user, due to the fact that user belongs to group. So I just mask out any bits that other has that group doesn't, so 0o757 turns into 0o755. You can still have 0o057, or 0o700 or whatever, it's just the case where other has bits that group doesn't that gets fixed up.

vtjnash · 2020-10-15T17:57:55Z

src/win/fs.c

+   * we're building, the `SetNamedSecurityInfoW()` function call below will
+   * place all DENY entries first, and all `ALLOW` entries second.  This


I think you mean SetEntriesInAcl, which we should perhaps just avoid due to this (like cygwin)

From my initial explorations, if we don't use SetEntriesInAcl, it's a lot more work. We have to manually merge entries and such that I would much prefer not to do. The Cygwin function to do this is many hundreds of lines of code; not counting their ACL datatype abstraction. I don't actually care about this case (others with more permissions than group) very much, so I'm alright with just not supporting it and doing something less-broken than before.

musm · 2020-12-28T15:50:55Z

Does this solve issues like the following:

mkdir("C:\\test")
chmod("C:\\test", 0o777)
rm("C:\\test") # currently a permission denied error

# On the other hand, without chmod
mkdir("C:\\test")
rm("C:\\test") # works

vtjnash · 2021-07-26T19:20:18Z

bump

vtjnash · 2022-07-12T18:24:59Z

bump

staticfloat · 2022-10-18T20:47:53Z

With the latest commit on windows:

mktempdir() do dir
    foo = joinpath(dir, "foo")
    @info("default")
    touch(foo)
    run(`icacls $(foo)`)

    for perm in (0o777, 0o757, 0o750, 0o600)
        @info(string(perm, base=8))
        chmod(foo, perm)
        run(`icacls $(foo)`)
        @show Sys.isexecutable(foo)
    end
end

Results in:

[ Info: default
C:\Users\Administrator\AppData\Local\Temp\jl_zAzid9\foo NT AUTHORITY\SYSTEM:(I)(F)
                                                        BUILTIN\Administrators:(I)(F)
                                                        DEBUGGING-AMDCI\Administrator:(I)(F)

Successfully processed 1 files; Failed processing 0 files
[ Info: 777
C:\Users\Administrator\AppData\Local\Temp\jl_zAzid9\foo BUILTIN\Administrators:(M,DC)
                                                        DEBUGGING-AMDCI\None:(M,DC)
                                                        Everyone:(M,DC)
                                                        NT AUTHORITY\SYSTEM:(M,DC)
                                                        DEBUGGING-AMDCI\Administrator:(M,DC)

Successfully processed 1 files; Failed processing 0 files
Sys.isexecutable(foo) = true
[ Info: 757
C:\Users\Administrator\AppData\Local\Temp\jl_zAzid9\foo BUILTIN\Administrators:(M,DC)
                                                        DEBUGGING-AMDCI\None:(RX,WA)
                                                        Everyone:(RX,WA)
                                                        NT AUTHORITY\SYSTEM:(RX,WA)
                                                        DEBUGGING-AMDCI\Administrator:(RX,WA)

Successfully processed 1 files; Failed processing 0 files
Sys.isexecutable(foo) = true
[ Info: 750
C:\Users\Administrator\AppData\Local\Temp\jl_zAzid9\foo BUILTIN\Administrators:(M,DC)
                                                        DEBUGGING-AMDCI\None:(RX,WA)
                                                        DEBUGGING-AMDCI\Administrator:(RX,WA)

Successfully processed 1 files; Failed processing 0 files
Sys.isexecutable(foo) = true
[ Info: 600
C:\Users\Administrator\AppData\Local\Temp\jl_zAzid9\foo BUILTIN\Administrators:(R,W,D,DC)

Successfully processed 1 files; Failed processing 0 files
Sys.isexecutable(foo) = false

Which I think all looks good to me. I also added some icacls calls to remove all ACE entries, hoping that this would cause the ACL to return NULL, and it continued to function just fine:

mktempdir() do dir
    foo = joinpath(dir, "foo")
    @info("default")
    touch(foo)
    run(`icacls $(foo)`)

    @info("NULL")
    run(`icacls $(foo) /inheritance:d`)
    run(`icacls $(foo) /remove "NT AUTHORITY\\SYSTEM"`)
    run(`icacls $(foo) /remove BUILTIN\\Administrators`)
    run(`icacls $(foo) /remove Administrator`)
    run(`icacls $(foo)`)

    for perm in (0o777, 0o757, 0o750, 0o600)
        @info(string(perm, base=8))
        chmod(foo, perm)
        run(`icacls $(foo)`)
        @show Sys.isexecutable(foo)
    end
end

[ Info: default
C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo NT AUTHORITY\SYSTEM:(I)(F)
                                                        BUILTIN\Administrators:(I)(F)
                                                        DEBUGGING-AMDCI\Administrator:(I)(F)

Successfully processed 1 files; Failed processing 0 files
[ Info: NULL
processed file: C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo
Successfully processed 1 files; Failed processing 0 files
processed file: C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo
Successfully processed 1 files; Failed processing 0 files
processed file: C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo
Successfully processed 1 files; Failed processing 0 files
processed file: C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo
Successfully processed 1 files; Failed processing 0 files
C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo 
Successfully processed 1 files; Failed processing 0 files
[ Info: 777
C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo BUILTIN\Administrators:(M,DC)
                                                        DEBUGGING-AMDCI\None:(M,DC)
                                                        Everyone:(M,DC)

Successfully processed 1 files; Failed processing 0 files
Sys.isexecutable(foo) = true
[ Info: 757
C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo BUILTIN\Administrators:(M,DC)
                                                        DEBUGGING-AMDCI\None:(RX,WA)
                                                        Everyone:(RX,WA)

Successfully processed 1 files; Failed processing 0 files
Sys.isexecutable(foo) = true
[ Info: 750
C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo BUILTIN\Administrators:(M,DC)
                                                        DEBUGGING-AMDCI\None:(RX,WA)

Successfully processed 1 files; Failed processing 0 files
Sys.isexecutable(foo) = true
[ Info: 600
C:\Users\Administrator\AppData\Local\Temp\jl_3cqGE6\foo BUILTIN\Administrators:(R,W,D,DC)

Successfully processed 1 files; Failed processing 0 files
Sys.isexecutable(foo) = false

vtjnash · 2022-10-19T15:41:32Z

CI is unhappy with this currently:

D:\a\libuv\libuv\src\win\fs.c(2448,3): warning C4013: 'build_access_struct' undefined; assuming extern returning int D:\a\libuv\libuv\src\win\fs.c(2518,37): error C2065: 'S_IWUSR': undeclared identifier D:\a\libuv\libuv\src\win\fs.c(2518,47): error C2065: 'S_IWGRP': undeclared identifier D:\a\libuv\libuv\src\win\fs.c(2518,56): error C2065: 'S_IWOTH': undeclared identifier D:\a\libuv\libuv\src\win\fs.c(2521,37): error C2065: 'S_IWUSR': undeclared identifier D:\a\libuv\libuv\src\win\fs.c(2521,47): error C2065: 'S_IWGRP': undeclared identifier D:\a\libuv\libuv\src\win\fs.c(2521,56): error C2065: 'S_IWOTH': undeclared identifier

vtjnash · 2022-10-21T13:30:37Z

we can merge once CI passes

staticfloat · 2022-11-30T19:26:05Z

I can't tell if CI is passing 😅

vtjnash · 2022-11-30T19:46:54Z

Fair point, but let's say at least "builds" even if it does not then pass all tests

staticfloat · 2022-12-01T18:07:16Z

It does build on our windows CI machines, which is where I tested it. What build status should I pay attention to here that says it doesn't build?

vtjnash · 2022-12-01T18:40:13Z

The console here for GitHub actions splits up the jobs by stage. FWIW, it is an MSVC thing, probably where the definitions are supposed to have a leading _ but where mingw-w64 adds extra ones without the underscore for simplicity

staticfloat · 2022-12-05T15:09:03Z

I tried using _mode_t, but it doesn't know that one either. I decided to just use DWORD, since this variable is completely of our own construction anyway.

vtjnash · 2023-01-11T16:55:35Z

still failing CI

D:\a\libuv\libuv\src\win\fs.c(2432,41): error C2065: 'S_IRWXU': undeclared identifier [D:\a\libuv\libuv\build\uv.vcxproj]
[104](https://github.com/JuliaLang/libuv/actions/runs/3621315770/jobs/6104606485#step:4:105)

staticfloat · 2023-01-12T16:38:02Z

Hmmm, should I #define those if they're missing?

vtjnash · 2023-01-12T23:50:06Z

yep

vtjnash · 2023-05-23T13:57:44Z

bump?

No longer shy away from using `AccessCheck()` for simple `W_OK`/`R_OK` checks; use it for all calls unconditionally.

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.

It's not needed to use `mode_t` here, and it just confuses VS.

vtjnash · 2024-02-09T14:31:29Z

bump?

vtjnash · 2025-01-17T16:43:18Z

bump?

staticfloat · 2025-01-19T04:30:27Z

What more needs to be done here? I #define'd the missing symbols, correct?

vtjnash · 2025-01-22T02:08:57Z

I was hoping you could tell me 😅. Is it ready?

staticfloat · 2025-02-10T15:15:48Z

Closed by #41

staticfloat requested a review from vtjnash October 15, 2020 01:45

staticfloat force-pushed the sf/acl_rebase branch from 191cb5e to 5efeb45 Compare October 15, 2020 02:41

vtjnash reviewed Oct 15, 2020

View reviewed changes

vtjnash changed the base branch from julia-uv2-1.39.0 to julia-uv2-1.42.0 July 26, 2021 19:19

vtjnash changed the base branch from julia-uv2-1.42.0 to julia-uv2-1.39.0 July 26, 2021 19:20

staticfloat force-pushed the sf/acl_rebase branch from 5efeb45 to 9759885 Compare October 18, 2022 18:47

staticfloat changed the base branch from julia-uv2-1.39.0 to julia-uv2-1.44.2 October 18, 2022 18:48

staticfloat force-pushed the sf/acl_rebase branch from b0b7947 to 7718e99 Compare December 5, 2022 15:08

vtjnash mentioned this pull request Dec 9, 2022

segfault in new fs__chmod code #23

Closed

stevengj mentioned this pull request Feb 6, 2023

add package crashes with Exception violation when I try to add any package JuliaLang/julia#41642

Open

staticfloat and others added 5 commits May 23, 2023 14:22

win,fs Use AccessCheck() for all access() calls

c9d68ff

No longer shy away from using `AccessCheck()` for simple `W_OK`/`R_OK` checks; use it for all calls unconditionally.

Fix null pointer exception reported in #23

3b2ec18

Apply "other" permissions to all groups we are not a part of

89c1b49

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.

Use DWORD instead of mode_t on Windows

0097e68

It's not needed to use `mode_t` here, and it just confuses VS.

Define S_IRWXU and others if not already defined.

4f92483

staticfloat force-pushed the sf/acl_rebase branch from 7718e99 to 4f92483 Compare May 23, 2023 14:31

staticfloat mentioned this pull request Jan 22, 2025

Address review comments on libuv patch #41

Merged

staticfloat closed this Feb 10, 2025

		* we're building, the `SetNamedSecurityInfoW()` function call below will
		* place all DENY entries first, and all `ALLOW` entries second. This

Uh oh!

Address review comments on libuv patch #16

Address review comments on libuv patch #16

Uh oh!

Conversation

staticfloat commented Oct 15, 2020

Uh oh!

vtjnash Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

staticfloat Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

musm commented Dec 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vtjnash commented Jul 26, 2021

Uh oh!

vtjnash commented Jul 12, 2022

Uh oh!

staticfloat commented Oct 18, 2022

Uh oh!

vtjnash commented Oct 19, 2022

Uh oh!

vtjnash commented Oct 21, 2022

Uh oh!

staticfloat commented Nov 30, 2022

Uh oh!

vtjnash commented Nov 30, 2022

Uh oh!

staticfloat commented Dec 1, 2022

Uh oh!

vtjnash commented Dec 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staticfloat commented Dec 5, 2022

Uh oh!

vtjnash commented Jan 11, 2023

Uh oh!

staticfloat commented Jan 12, 2023

Uh oh!

vtjnash commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vtjnash commented May 23, 2023

Uh oh!

vtjnash commented Feb 9, 2024

Uh oh!

vtjnash commented Jan 17, 2025

Uh oh!

staticfloat commented Jan 19, 2025

Uh oh!

vtjnash commented Jan 22, 2025

Uh oh!

staticfloat commented Feb 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

musm commented Dec 28, 2020 •

edited

Loading

vtjnash commented Dec 1, 2022 •

edited

Loading

vtjnash commented Jan 12, 2023 •

edited

Loading