Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix artifact directories not having traversal permissions #4075

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

staticfloat
Copy link
Member

It turns out that Windows requires the executable bit set if the BYPASS_TRAVERSE_CHECKING privilege is not attached to a user's account. It's simply more correct to ensure that our directories have this bit set, and unfortunately our filemode() function call is not complete on Windows and does not include this bit, so we manually add it in on Windows.

staticfloat added a commit to JuliaLang/julia that referenced this pull request Nov 5, 2024
It turns out that Windows requires the executable bit set if the
`BYPASS_TRAVERSE_CHECKING` privilege is not attached to a user's
account.  It's simply more correct to ensure that our directories have
this bit set, and unfortunately our `filemode()` function call is
not complete on Windows and does not include this bit, so we manually
add it in on Windows.
@giordano
Copy link
Contributor

giordano commented Nov 5, 2024

Is this related to JuliaLang/julia#52272?

@staticfloat
Copy link
Member Author

It may not fix all cases, but it is one way such issues arise.

@StefanKarpinski
Copy link
Member

Should we also be setting this bit on directories in Tar.extract?

@staticfloat
Copy link
Member Author

In my tests, Tar.extract didn’t set any permissions on directories on windows.

@ViralBShah
Copy link
Member

Shall we merge this and have it backported to the upcoming 1.10.7 (if there is still time?) and 1.11 as well?

@davidanthoff
Copy link

Why are we still modifying file permissions on Windows at all? I'm still completely convinced that we should do absolutely nothing to modify file permissions on Windows systems, see JuliaLang/julia#52272 (comment) for my explanation of that.

@topolarity
Copy link
Member

Libuv seems to agree with you: https://github.com/libuv/libuv/blob/31ea3411ccab6db63a3f573523be61f80f984d50/src/win/fs.c#L1840-L1849

I'm a bit surprised this even does anything.. I think libuv calls _wchmod, which is documented as ignoring anything that's not _S_IWRITE or _S_IREAD (in particular _S_IEXEC is being set here)

# If this is Windows, ensure the directory mode is executable,
# as `filemode()` is incomplete. Some day, that may not be the
# case, there exists a test that will fail if this is changes.
new_path_mode |= 0o001
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, shouldn't this actually be 0o111?

@staticfloat
Copy link
Member Author

I continue to believe that it's better to have chmod() do something than nothing, and that bugs in implementation should just be fixed so the behavior is as close to reasonable as possible. However, I also understand the argument that this is an unusual position for opensource-on-windows-software to take, and I could see the argument for reverting this behavior in future releases of Julia.

However, at least for the present releases, I think this change is purely an improvement. (filemode() not returning +x on Windows is pretty clearly a bug, working around that seems like a fine thing to do. Alternatively, fixing filemode() could also be a good thing to do, but that's more invasive.)

@topolarity
Copy link
Member

I continue to believe that it's better to have chmod() do something than nothing...
However, at least for the present releases, I think this change is purely an improvement.

I'm confused.. maybe I chased through the source code wrong, but based on the libuv implementation it really looks like the chmod call (the one implemented by Microsoft) that this is passed to ought to do nothing with the new flag.

@staticfloat
Copy link
Member Author

staticfloat commented Dec 2, 2024

I'm confused.. maybe I chased through the source code wrong, but based on the libuv implementation it really looks like the chmod call (the one implemented by Microsoft) that this is passed to ought to do nothing with the new flag.

You want to take a look at this function, our fork has diverged from upstream libuv on this, because of work that Jameson and I did.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants