Skip to content
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

Avoid modifying the process global environment (not thread safe) #83557

Open
gpshead opened this issue Jan 18, 2020 · 8 comments
Open

Avoid modifying the process global environment (not thread safe) #83557

gpshead opened this issue Jan 18, 2020 · 8 comments
Labels
3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-subinterpreters type-bug An unexpected behavior, bug, or error

Comments

@gpshead
Copy link
Member

gpshead commented Jan 18, 2020

BPO 39376
Nosy @gpshead, @ericsnowcurrently

Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2020-01-18.00:47:44.215>
labels = ['expert-subinterpreters', 'interpreter-core', '3.9', 'type-crash']
title = 'Avoid modifying the process global environment (not thread safe)'
updated_at = <Date 2021-04-16.00:43:07.242>
user = 'https://github.com/gpshead'

bugs.python.org fields:

activity = <Date 2021-04-16.00:43:07.242>
actor = 'vstinner'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core', 'Subinterpreters']
creation = <Date 2020-01-18.00:47:44.215>
creator = 'gregory.p.smith'
dependencies = []
files = []
hgrepos = []
issue_num = 39376
keywords = []
message_count = 2.0
messages = ['360222', '360225']
nosy_count = 2.0
nosy_names = ['gregory.p.smith', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'needs patch'
status = 'open'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue39376'
versions = ['Python 3.9']

@gpshead
Copy link
Member Author

gpshead commented Jan 18, 2020

For more context, see https://bugs.python.org/issue39375 which seeks to document the existing caveats.

POSIX lacks any APIs to access the process global environment in a thread safe manner. Given this, we could _consider_ preventing os.putenv() and os.environ[x] = y assignment from actually modifying the process global environment. They'd save their changes in our local os.environ underlying dict, set a flag that it was modified, but not modify the global.

This would be a visible behavior change and break _some_ class of code. :/

Our stdlib codepaths that launch a new process on POSIX could be modified to to always pass our a newly constructed envp from os.environ to exec/spawn APIs. The os.system() API would need to stop using the POSIX system() API call in order for that to work.

Downside API breakage: Extension module modifications to the environment would not be picked up by Python interpreter launched subprocesses. How much of a problem would that be in practice?

We may decide to close this as infeasible and just stick with the documentation of the sorry state of POSIX and not attempt to offer any safe non-crash-possible workarounds.

@gpshead gpshead added 3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump labels Jan 18, 2020
@ericsnowcurrently
Copy link
Member

+1

This has impact on subinterpreters once they stop sharing the GIL. (It's already on my list of global resources that need better protection.)

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@ericsnowcurrently ericsnowcurrently added type-bug An unexpected behavior, bug, or error and removed type-crash A hard crash of the interpreter, possibly with a core dump labels Jun 18, 2024
@ericsnowcurrently
Copy link
Member

@colesbury, is this something that needs to be addressed for free-threaded builds?

@gpshead
Copy link
Member Author

gpshead commented Jun 18, 2024

given modifying the C/kernel provided environment blob is already broken regardless of free-threaded builds, I doubt it needs special attention beyond making sure the existing os.putenv(x, y) and os.environ[x] = y documentation calls it out.

those APIs haven't been compatible with the modern world for decades.

@gpshead
Copy link
Member Author

gpshead commented Jun 18, 2024

If we went with my idea in the opening message of tracking local modifications, it might be a challenge. But my gut feeling is that we just won't bother doing this? I don't recall the last time I've seen this come up as an issue. Hopefully that means people just don't do it and pass new env= mappings to subprocesses?

@colesbury
Copy link
Contributor

I agree with what @gpshead wrote -- I think we should document it, but I don't think there's a good fix for it at the Python level.

The same problem affects other languages and runtimes:

Rachel Kroll had a blog post ~7 years ago about setenv that concludes with:

None of this is new, but we do re-discover it roughly every five years.
See you in 2022.

I expect we'll have users run into this problem every so often, even with updated docs. It's too bad because I think the technical fix for glibc, etc. is not too complex.

@gpshead
Copy link
Member Author

gpshead commented Jun 20, 2024

While I mostly like to pretend putenv() or modifying main's char **environ doesn't exist and the world has largely adopted the same mentality... The code I added recently to try and detect a multithreaded process to warn people who were forking could be generalized and reused here for an environment modification warning when threads exist.

https://github.com/python/cpython/blob/main/Modules/posixmodule.c#L7884 warn_about_fork_with_threads()

@tbu-
Copy link

tbu- commented Jun 27, 2024

Permalink:

warn_about_fork_with_threads(const char* name)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.9 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-subinterpreters type-bug An unexpected behavior, bug, or error
Projects
Status: Todo
Development

No branches or pull requests

5 participants