Skip to content

gh-111841: Fix os.putenv() and os.unsetenv() with embedded NUL on Windows #111842

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

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Nov 8, 2023

@serhiy-storchaka
Copy link
Member Author

It also makes test_posix running on Windows. Interesting that only two test_posix tests failed on Windows: the one for testing this bug (which was fixed on other platforms), and the other that tests Posix-specific constants.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I don't understand this change. PyUnicode_AsWideCharString() should already implements the check and test_os has tests on embedded null characters in the env var name and in the env var value.

import posix
except ImportError:
try:
import nt as posix
Copy link
Member

Choose a reason for hiding this comment

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

That's strange. For me, "test_posix" are tests for the POSIX platforms: all platforms but Windows.

If you want to write tests on alls platform including Windows, add tests to test_os, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

They are already written, and it turned out that they are compatible with Windows.

In the past there were several platform specific implementations: posix, nt, dos, os2, mac, ce. But posix and nt became so close that they are now build from the same source, and other implementations are gone.

@vstinner
Copy link
Member

vstinner commented Nov 9, 2023

PyUnicode_AsWideCharString() should already implements the check and test_os has tests on embedded null characters in the env var name and in the env var value.

I looked at the code and played with Python on Windows:

  • PyUnicode_AsWideCharString(str, NULL) rejects embedded null characters.
  • PyUnicode_AsWideCharString(str, &size) does not reject embedded null characters.
  • win32_putenv() doesn't use PyUnicode_AsWideCharString(str, NULL) but PyUnicode_AsWideCharString(str, &size).
  • test_os checks if os.putenv() raises ValueError or OSError when the env var name contains an embedded null character.

Oh ok, now I get it.

Well, currently, OSError is raised at least.

@vstinner
Copy link
Member

vstinner commented Nov 9, 2023

If you want to run test_posix on Windows, can you please do it in a separated PR? This PR is about embedded null characters and putenv(). Running test_posix on Windows is a significant change.

@serhiy-storchaka
Copy link
Member Author

If you want to run test_posix on Windows, can you please do it in a separated PR?

Done. See #111913.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM

@miss-islington-app
Copy link

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@serhiy-storchaka serhiy-storchaka deleted the win-os-putenv-embedded-nul branch November 10, 2023 16:42
@bedevere-app
Copy link

bedevere-app bot commented Nov 10, 2023

GH-111966 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Nov 10, 2023
@bedevere-app
Copy link

bedevere-app bot commented Nov 10, 2023

GH-111967 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Nov 10, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 10, 2023
…on Windows (pythonGH-111842)

(cherry picked from commit 0b06d24)

Co-authored-by: Serhiy Storchaka <[email protected]>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 10, 2023
…on Windows (pythonGH-111842)

(cherry picked from commit 0b06d24)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit that referenced this pull request Nov 11, 2023
… on Windows (GH-111842) (GH-111966)

(cherry picked from commit 0b06d24)

Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 RHEL8 LTO + PGO 3.12 has failed when building commit 3bd8b74.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1168/builds/411) and take a look at the build logs.
  4. Check if the failure is related to this commit (3bd8b74) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1168/builds/411

Summary of the results of the build (if available):

Click to see traceback logs
remote: Enumerating objects: 13, done.        
remote: Counting objects:   8% (1/12)        
remote: Counting objects:  16% (2/12)        
remote: Counting objects:  25% (3/12)        
remote: Counting objects:  33% (4/12)        
remote: Counting objects:  41% (5/12)        
remote: Counting objects:  50% (6/12)        
remote: Counting objects:  58% (7/12)        
remote: Counting objects:  66% (8/12)        
remote: Counting objects:  75% (9/12)        
remote: Counting objects:  83% (10/12)        
remote: Counting objects:  91% (11/12)        
remote: Counting objects: 100% (12/12)        
remote: Counting objects: 100% (12/12), done.        
remote: Compressing objects:   8% (1/12)        
remote: Compressing objects:  16% (2/12)        
remote: Compressing objects:  25% (3/12)        
remote: Compressing objects:  33% (4/12)        
remote: Compressing objects:  41% (5/12)        
remote: Compressing objects:  50% (6/12)        
remote: Compressing objects:  58% (7/12)        
remote: Compressing objects:  66% (8/12)        
remote: Compressing objects:  75% (9/12)        
remote: Compressing objects:  83% (10/12)        
remote: Compressing objects:  91% (11/12)        
remote: Compressing objects: 100% (12/12)        
remote: Compressing objects: 100% (12/12), done.        
remote: Total 13 (delta 0), reused 7 (delta 0), pack-reused 1        
From https://github.com/python/cpython
 * branch                  3.12       -> FETCH_HEAD
Auto packing the repository in background for optimum performance.
See "git help gc" for manual housekeeping.
Note: switching to '3bd8b743305f0d788302df3ff08c2bb7eb0dd297'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by switching back to a branch.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -c with the switch command. Example:

  git switch -c <new-branch-name>

Or undo this operation with:

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at 3bd8b74330 [3.12] gh-111841: Fix os.putenv() and os.unsetenv() with embedded NUL on Windows (GH-111842) (GH-111966)
Switched to and reset branch '3.12'

find: ‘./.git/objects/1c’: No such file or directory
find: ‘./.git/objects/20’: No such file or directory
find: ‘./.git/objects/21’: No such file or directory
find: ‘./.git/objects/22’: No such file or directory
find: ‘./.git/objects/24’: No such file or directory
find: ‘./.git/objects/27’: No such file or directory
find: ‘./.git/objects/23’: No such file or directory
find: ‘./.git/objects/1b’: No such file or directory
find: ‘./.git/objects/1a’: No such file or directory
find: ‘./.git/objects/1d’: No such file or directory
find: ‘./.git/objects/28’: No such file or directory
make[2]: [Makefile:2783: pycremoval] Error 1 (ignored)
find: ‘./.git/objects/47’: No such file or directory
find: ‘./.git/objects/46’: No such file or directory
find: ‘./.git/objects/49’: No such file or directory
find: ‘./.git/objects/44’: No such file or directory
find: ‘./.git/objects/48’: No such file or directory
find: ‘./.git/objects/45’: No such file or directory
make[2]: [Makefile:2784: pycremoval] Error 1 (ignored)
find: ‘./.git/objects/68’: No such file or directory
find: ‘./.git/objects/6a’: No such file or directory
find: ‘./.git/objects/6f’: No such file or directory
find: ‘./.git/objects/6c’: No such file or directory
find: ‘./.git/objects/6e’: No such file or directory
find: ‘./.git/objects/70’: No such file or directory
find: ‘./.git/objects/69’: No such file or directory
make[2]: *** [Makefile:2802: clean-retain-profile] Error 1
make[1]: *** [Makefile:794: profile-clean-stamp] Error 2
make: *** [Makefile:813: profile-run-stamp] Error 2

find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make: [Makefile:2820: clean-retain-profile] Error 1 (ignored)

serhiy-storchaka added a commit that referenced this pull request Nov 11, 2023
… on Windows (GH-111842) (GH-111967)

(cherry picked from commit 0b06d24)

Co-authored-by: Serhiy Storchaka <[email protected]>
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants