-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
gh-133562: disable security descriptor handling on unsupported WinAPI partitions #133563
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
Conversation
Is it the This change was in response to a vulnerability report (mkdtemp not meeting the required permissions when run as a non-user account), so skipping it for contexts where it's entirely not possible would be okay. I'd prefer to keep it in if we can, though. |
Modules/posixmodule.c
Outdated
@@ -5726,6 +5726,7 @@ os_mkdir_impl(PyObject *module, path_t *path, int mode, int dir_fd) | |||
|
|||
#ifdef MS_WINDOWS | |||
Py_BEGIN_ALLOW_THREADS | |||
#if defined(MS_WINDOWS_DESKTOP) || defined(MS_WINDOWS_SYSTEM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you should explicitly use MS_WINDOWS_GAMES or comment that it is for XBox build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We decided last time around to use inclusive filters rather than exclusion ones - we know the platforms where the API is available, and it won't be removed from those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explanation!
Yes it's |
We should at least try building up the SD manually before just disabling it. According to Copilot, this code should do it - could you try integrating it in and see (I'm pretty sure the memory allocations can also be simplified for our case, perhaps with an inline struct definition)?
|
looks like just the descriptor is available:
are all restricted to |
looking at the partition definition this means it's available for everything except |
Okay, in that case, let's just exclude the functionality. But I'll add some clarifying comments to go with it, since this does look like a security risk (apart from the limited context in which it applies) |
Misc/NEWS.d/next/Library/2025-05-07-09-02-19.gh-issue-133562.lqqNW1.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Steve Dower <steve.dower@microsoft.com>
…qqNW1.rst Co-authored-by: Steve Dower <steve.dower@microsoft.com>
I assume the CI failures are unrelated |
Hopefully they are, I'll merge from main in case they've been fixed somewhere eles. |
ping @zooba the ci does now pass. Also I do have three more PRs in regards to compiling on xbox:
With those three additional changes + the mimalloc change (I will try to get this merged upstream first), I have Python 3.13 running on the xbox again. |
Looks like no actual tests were run, I'll update again and see if that helps. |
@zooba you added the handling for mode
0o700
. Is it fine to just ignore this mode, or would we need to error out in this case?