Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork34.1k
gh-137335: Remove use ofmktemp() inasyncio.windows_utils#137333
gh-137335: Remove use ofmktemp() inasyncio.windows_utils#137333lighting9999 wants to merge 8 commits intopython:mainfromlighting9999:path-1
mktemp() inasyncio.windows_utils#137333Conversation
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
StanFromIreland commentedAug 3, 2025
Hello, thanks for your contribution. Yes, |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
lighting9999 commentedAug 3, 2025
@StanFromIreland I create issue at#137335, welcome to change! |
picnixz left a comment
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.
The title is wrong. This doesn't deprecate mktemp() and the fix does not take into account the temporary directory anymore which is wrong (now the address file will be directly in\\.pipe\... instead of the temporary directory described by the user). Please usetempfile.mkstemp instead.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
picnixz commentedAug 3, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'm tagging this as DO-NOT-MERGE until we prove that there are indeed real possible issues here. I honestly don't think there are possible issues in practice as we are using a custom prefix here, and if someone beats us in creating the file, then ... that's not an issue IMO since that means it must be crafted internally or intentionally as we are also using the process ID to reduce collisions. In the first case, that's an issue to solve on our side but I can't find something similar. In the second case, we would just use that bad file but the files are opened in generic read/write mode (read-only if not duplex). I'm not an expert in the Windows API though so I'd like concrete evidence of a possible security issue here. |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
lighting9999 commentedAug 3, 2025
I wrote a POC and found out that it had the possibility of denial of service. |
StanFromIreland commentedAug 3, 2025
Please do not use theUpdate Branch button unless necessary (e.g. fixing conflicts, jogging the CI, or very old PRs) as it uses valuable resources. For more information see thedevguide. |
lighting9999 commentedAug 3, 2025
ok |
Most changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
picnixz left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
The title hasn't been changed, and there is still a similar construction in multiprocessing.utils. The NEWS entry is still missing.
mktemp() inasyncio.windows_utilsMost changes to Pythonrequire a NEWS entry. Add one using theblurb_it web app or theblurb command-line tool. If this change has little impact on Python users, wait for a maintainer to apply the |
lighting9999 commentedAug 4, 2025
multiprocessing.utils exists in that module or in that file? |
StanFromIreland commentedAug 4, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
It is in |
lighting9999 commentedAug 4, 2025
ok |
picnixz commentedAug 4, 2025
Sorry, I actually meant |
lighting9999 commentedAug 5, 2025
oh....... |
picnixz commentedAug 5, 2025
However, as we said on the issue, the problem might be fundamentally different so Serhiy is taking over the issue. |
Uh oh!
There was an error while loading.Please reload this page.
matemp() function is unsafe,so I use safety function.
bandit tool:
tempfile.mktemp()for creating named pipes on Windows #137335