Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-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.
This comment has been minimized.
This comment has been minimized.
the-knights-who-say-ni
commented
Aug 23, 2019
|
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! |
benoithudson
changed the title
This comment has been minimized.
This comment has been minimized.
|
@ronaldoussoren should review this |
|
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
This comment has been minimized.
This comment has been minimized.
|
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. |
This comment has been minimized.
This comment has been minimized.
|
@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. |
|
LGTM. |
This comment has been minimized.
This comment has been minimized.
|
I can't add labels; if someone can add |
|
LGTM |
This comment has been minimized.
This comment has been minimized.
|
That's annoying: "squash and merge" fails for me at the moment, and twice in a row ("Merge attempt failed"). Will try again later. |
This comment has been minimized.
This comment has been minimized.
|
Any luck trying again? |
This comment has been minimized.
This comment has been minimized.
|
The change looks perfectly safe to me. I cannot test it, but I trust @benoithudson managed to confirm that his change fix his crash. |
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Dec 6, 2019
|
Thanks @benoithudson for the PR, and @vstinner for merging it |
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>
This comment has been minimized.
This comment has been minimized.
bedevere-bot
commented
Dec 6, 2019
|
GH-17488 is a backport of this pull request to the 3.8 branch. |
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Dec 6, 2019
|
Sorry, @benoithudson and @vstinner, I could not cleanly backport this to |
This comment has been minimized.
This comment has been minimized.
miss-islington
commented
Dec 6, 2019
|
Sorry @benoithudson and @vstinner, I had trouble checking out the |
This comment has been minimized.
This comment has been minimized.
|
@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. |
This comment has been minimized.
This comment has been minimized.
|
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>
bpo-37931: Fix crash on OSX re-initializing os.environ (pythonGH-15428)
benoithudson commentedAug 23, 2019
•
edited by bedevere-bot
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