[Security] support.args_from_interpreter_flags() doesn't inherit -I (isolated) flag

[vstinner]

BPO	34812
Nosy	@vstinner, @miss-islington, @tirkarthi, @danishprakash
PRs	bpo-34812: subprocess._args_from_interpreter_flags(): add isolated #10675 [3.7] bpo-34812: subprocess._args_from_interpreter_flags(): add isolated (GH-10675) #10684 [3.6] bpo-34812: subprocess._args_from_interpreter_flags(): add isolated (GH-10675) #10688

^{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 = <Date 2018-11-23.18:03:21.540>
created_at = <Date 2018-09-26.16:07:03.845>
labels = ['type-security', '3.8', '3.7', 'library']
title = "[Security] support.args_from_interpreter_flags() doesn't inherit -I (isolated) flag"
updated_at = <Date 2018-11-23.18:03:21.538>
user = 'https://github.com/vstinner'

bugs.python.org fields:

activity = <Date 2018-11-23.18:03:21.538>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2018-11-23.18:03:21.540>
closer = 'vstinner'
components = ['Library (Lib)']
creation = <Date 2018-09-26.16:07:03.845>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 34812
keywords = ['patch']
message_count = 19.0
messages = ['326478', '326669', '326787', '326790', '326792', '326875', '326876', '326879', '327099', '327280', '327289', '327318', '327321', '330295', '330325', '330340', '330344', '330351', '330352']
nosy_count = 4.0
nosy_names = ['vstinner', 'miss-islington', 'xtreak', 'danishprakash']
pr_nums = ['10675', '10684', '10688']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue34812'
versions = ['Python 3.6', 'Python 3.7', 'Python 3.8']

[vstinner]

The support.args_from_interpreter_flags() function recreates Python command line arguments from sys.flags, but it omits -I (sys.flags.isolated).

Because of that, "./python -I -m test ..." behaves differently than "./python -I -m test -j0 ...":
https://bugs.python.org/issue28655#msg326477

[tirkarthi]

Thanks Victor for the details. Can this be classified as an easy issue? I guess the fix will be as below :

1. Add an entry for '-I' at

   cpython/Lib/subprocess.py

   Line 260 in 4b430e5

   flag_opt_map = {

2. Add a test for '-I' at

   cpython/Lib/test/test_support.py

   Line 472 in 4b430e5

   def test_args_from_interpreter_flags(self):

   .
   The only thing here is that '-I' returns '-s -E -I' unlike other options where args can be used for comparison logic in check_options. check_options should be changed so that this can take given args and the expected args for comparison to accommodate -I. Maybe there is a better way?

Off topic : I don't know why '-I' is not documented as sys.flags.isolated at https://docs.python.org/3.7/library/sys.html#sys.flags . Maybe I will open up a separate issue for this?

[vstinner]

In the C code, sys.flags.isolated clearly documented as linked to the -I option:

static PyStructSequence_Field flags_fields[] = {
{"debug", "-d"},
{"inspect", "-i"},
{"interactive", "-i"},
{"optimize", "-O or -OO"},
{"dont_write_bytecode", "-B"},
{"no_user_site", "-s"},
{"no_site", "-S"},
{"ignore_environment", "-E"},
{"verbose", "-v"},
/* {"unbuffered", "-u"}, */
/* {"skip_first", "-x"}, */
{"bytes_warning", "-b"},
{"quiet", "-q"},
{"hash_randomization", "-R"},
{"isolated", "-I"},
{"dev_mode", "-X dev"},
{"utf8_mode", "-X utf8"},
{0}
};

The only thing here is that '-I' returns '-s -E -I' unlike other options where args can be used for comparison logic in check_options.

I expect to get:

$ python3 -I -c 'import subprocess; print(subprocess._args_from_interpreter_flags())'
['-I']

instead of:

['-s', '-E']

-I is different from -s -E: it also avoids to add the script directory or an empty string to sys.path.

[tirkarthi]

Thanks Victor for the details.

In the C code, sys.flags.isolated clearly documented as linked to the -I option:

With respect to documentation I was talking about '-I' not being documented in the table at https://docs.python.org/3.7/library/sys.html#sys.flags though it's present in the C code and in sys.flags.isolated.

-I is different from -s -E: it also avoids to add the script directory or an empty string to sys.path.

'-I' also implies '-s -E' and hence adding isolated to args_from_interpreter_flags will also return ['-s', '-E', '-I'] as output and hence I suggested modifying the comparison logic.

# Since '-I' implies '-s' and '-E' those flags are also set returning '-s -E -I'

./python.exe --help | rg '\-I'
-I : isolate Python from the user's environment (implies -E and -s)

./python.exe -I -c 'import sys; print(sys.flags)'
sys.flags(debug=0, inspect=0, interactive=0, optimize=0, dont_write_bytecode=0, no_user_site=1, no_site=0, ignore_environment=1, verbose=0, bytes_warning=0, quiet=0, hash_randomization=1, isolated=1, dev_mode=False, utf8_mode=0)

# patching args_from_interpreter_flags to support '-I' would return below

./python.exe -I -c 'import subprocess; print(subprocess._args_from_interpreter_flags())'
['-s', '-E', '-I']

Thanks

[vstinner]

./python.exe -I -c 'import subprocess; print(subprocess._args_from_interpreter_flags())'
['-s', '-E', '-I']

This looks wrong, I would prefer to only get ['-I'].

[danishprakash]

With respect to documentation I was talking about '-I' not being documented in the table at https://docs.python.org/3.7/library/sys.html#sys.flags though it's present in the C code and in sys.flags.isolated.

Thanks for bringing this up Karthikeyan, however, could there be another reason why -I would be left out. Also, have you filed an issue for this?

Also, Victor and Karthikeyan, since this issue has been categorized as an easy issue, I would like to fix this if none of you have started working on this.

[tirkarthi]

Thanks for bringing this up Karthikeyan, however, could there be another reason why -I would be left out. Also, have you filed an issue for this?

I couldn't see any related issue for this though the table was changed in 3.7.0

Also, Victor and Karthikeyan, since this issue has been categorized as an easy issue, I would like to fix this if none of you have started working on this.

I am not working on this. Feel free to pick it up.

[danishprakash]

Thank you Karthikeyan, I'm going to take care of both of these issues.

[danishprakash]

Linking this1 here in case someone else stumbles upon this thread. I've created an issue and a PR for the documentation issue regarding the absence of -I flag from the sys.flags table which came into picture from the discussions in this thread.

[danishprakash]

I expect to get: ['-I'] instead of ['-s', '-E', '-I']

From what I understand, this can be done in one of two ways. First, we could edit

cpython/Modules/main.c

Line 430 in ad73a9c

case 'I':

and not incrementing -s and -E. But I believe this would have consequences that I'm unable to think of right now, so I'd like some inputs on this.

Secondly, we could handle this condition in _args_from_interpreted_flags() itself but that might be looked upon as bad practice.

[tirkarthi]

From what I understand, this can be done in one of two ways. First, we could edit

cpython/Modules/main.c

Line 430 in ad73a9c

case 'I':

and not incrementing -s and -E. But I believe this would have consequences that I'm unable to think of right now, so I'd like some inputs on this.

As in the docs for -I it implies -s and -E so removing the increment is not a good solution in my opinion and will break code.

I don't know how this can be handled since -I sets -s and -E implicitly and _args_from_interpreted_flags just looks for the set flag. This could also get a little complex if we remove -s and -E based on -I since one might pass -I and -s. Maybe we can do an intersection of the command line arguments passes and the set bits in _args_from_interpreted_flags so that only -I remains? Victor prefers -I only and maybe has an approach to solve this?

[danishprakash]

You're right Karthikeyan, although I personally think that returning ['-s', '-E', '-I'] should be a plausible solution here since it has been stated explicitly that it implies '-s' and '-E' but I'm still waiting for what Victor has to say on this.

The only thing here is that '-I' returns '-s -E -I' unlike other options where args can be used for comparison logic in check_options.

Karthikeyan, do you happen to have a use case where this might come into action?

[tirkarthi]

> The only thing here is that '-I' returns '-s -E -I' unlike other options where args can be used for comparison logic in check_options.

Karthikeyan, do you happen to have a use case where this might come into action?

I don't have a use case in mind. The comment was that returning '-s -E -I' would need the helper function used in the test to be changed.

Thanks

[danishprakash]

Sorry for bumping this thread but Victor, could you please share your inputs on this if you have the time for it, thanks.

[vstinner]

I tried to explain how to fix the bug, but nobody came up with a working change 2 months, so I wrote the PR myself. It's an important security issue, since the function is used by multiprocessing and distutils modules to spawn new child processes.

[vstinner]

New changeset 9de3632 by Victor Stinner in branch 'master':
bpo-34812: subprocess._args_from_interpreter_flags(): add isolated (GH-10675)
9de3632

[miss-islington]

New changeset 01e5799 by Miss Islington (bot) in branch '3.7':
bpo-34812: subprocess._args_from_interpreter_flags(): add isolated (GH-10675)
01e5799

[vstinner]

New changeset cc0e0a2 by Victor Stinner in branch '3.6':
bpo-34812: subprocess._args_from_interpreter_flags(): add isolated (GH-10675) (GH-10688)
cc0e0a2

[vstinner]

Ok, the bug is now fixed in Python 3.6, 3.7 and master branches ;-)