bpo-37931: crash on OSX re-initializing os.environ #15428
Conversation
On most platforms, the `environ` symbol is accessible everywhere. In a dylib on OSX, it's not easily accessible, you need to find it with _NSGetEnviron. The code was caching the *value* of environ. But a setenv can change the value, leaving garbage at the old value. Fix: don't cache the value of environ, just read it every time.
|
Hello, and thanks for your contribution! I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA). Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue. You can check yourself to see if the CLA has been received. Thanks again for your contribution, we look forward to reviewing it! |
|
@ronaldoussoren should review this |
ronaldoussoren
left a comment
There was a problem hiding this comment.
The patch looks good, but its possible to centralise the macOS specific code a bit more, see my comments below.
Misc/NEWS.d/next/macOS/2019-08-23-12-14-34.bpo-37931.goYgQj.rst
Outdated
Show resolved
Hide resolved
|
How does backporting work -- do I prepare a new PR for those or does someone else take care of it? I'd ideally like to backport to 2.7 since that's where I actually hit the bug. This part of the codebase hasn't changed in a long time. |
|
@benoithudson, backporting is attempted automatically when a core developer adds the appropriate "needs backport to ..." labels to the PR, as @ronaldoussoren has done. It could be considered for 2.7. |
|
I can't add labels; if someone can add |
|
That's annoying: "squash and merge" fails for me at the moment, and twice in a row ("Merge attempt failed"). Will try again later. |
|
Any luck trying again? |
|
The change looks perfectly safe to me. I cannot test it, but I trust @benoithudson managed to confirm that his change fix his crash. |
|
Thanks @benoithudson for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7, 3.8. |
On most platforms, the `environ` symbol is accessible everywhere. In a dylib on OSX, it's not easily accessible, you need to find it with _NSGetEnviron. The code was caching the *value* of environ. But a setenv() can change the value, leaving garbage at the old value. Fix: don't cache the value of environ, just read it every time. (cherry picked from commit 723f71a) Co-authored-by: Benoit Hudson <benoit@imgspc.com>
|
GH-17488 is a backport of this pull request to the 3.8 branch. |
|
Sorry, @benoithudson and @vstinner, I could not cleanly backport this to |
|
Sorry @benoithudson and @vstinner, I had trouble checking out the |
|
@benoithudson: The automated backport to 2.7 and 3.7 failed. Would you mind to attempt to backport manually the change starting with "git cherry-pick -x 723f71a" or use cherry_picker, as explained in previous comments. |
|
Sure, I'll try to do that this weekend. |
On most platforms, the `environ` symbol is accessible everywhere. In a dylib on OSX, it's not easily accessible, you need to find it with _NSGetEnviron. The code was caching the *value* of environ. But a setenv() can change the value, leaving garbage at the old value. Fix: don't cache the value of environ, just read it every time. (cherry picked from commit 723f71a) Co-authored-by: Benoit Hudson <benoit@imgspc.com>
On most platforms, the `environ` symbol is accessible everywhere. In a dylib on OSX, it's not easily accessible, you need to find it with _NSGetEnviron. The code was caching the *value* of environ. But a setenv() can change the value, leaving garbage at the old value. Fix: don't cache the value of environ, just read it every time.
On most platforms, the
environsymbol is accessible everywhere.In a dylib on OSX, it's not easily accessible, you need to find it with
_NSGetEnviron.
The code was caching the value of environ. But a setenv can change the value,
leaving garbage at the old value. Fix: don't cache the value of environ, just
read it every time.
https://bugs.python.org/issue37931