10000 bpo-45445: Fail if an invalid X-option is provided in the command line by pablogsal · Pull Request #28823 · python/cpython · GitHub
[go: up one dir, main page]

Skip to content

bpo-45445: Fail if an invalid X-option is provided in the command line #28823

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
merged 6 commits into from
Oct 13, 2021
4 changes: 2 additions & 2 deletions Doc/library/sys.rst
Original file line number Diff line number Diff line change
Expand Up @@ -1725,13 +1725,13 @@ always available.

.. code-block:: shell-session

$ ./python -Xa=b -Xc
$ ./python -Xpycache_prefix=some_path -Xdev
Python 3.2a3+ (py3k, Oct 16 2010, 20:14:50)
[GCC 4.4.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys
>>> sys._xoptions
{'a': 'b', 'c': True}
{'pycache_prefix': 'some_path', 'dev': True}

.. impl-detail::

Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_audit.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
class AuditTest(unittest.TestCase):
def do_test(self, *args):
with subprocess.Popen(
[sys.executable, "-X utf8", AUDIT_TESTS_PY, *args],
[sys.executable, "-Xutf8", AUDIT_TESTS_PY, *args],
Copy link
Member Author
@pablogsal pablogsal Oct 12, 2021

Choose a reason for hiding this comment

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

Bonus! This was a bug because the Xoption was provided as utf8 (with space) and not utf8 so it was silently ignored :(

@vstinner @zooba

Copy link
Member

Choose a reason for hiding this comment

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

Oh, or you can write "-X", "utf8". I suppose that both work.

encoding="utf-8",
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
Expand All @@ -32,7 +32,7 @@ def do_test(self, *args):
def run_python(self, *args):
events = []
with subprocess.Popen(
[sys.executable, "-X utf8", AUDIT_TESTS_PY, *args],
[sys.executable, "-Xutf8", AUDIT_TESTS_PY, *args],
encoding="utf-8",
stdout=subprocess.PIPE,
stderr=subprocess.PIPE,
Expand Down
13 changes: 11 additions & 2 deletions Lib/test/test_cmd_line.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,17 @@ def get_xoptions(*args):
opts = get_xoptions()
self.assertEqual(opts, {})

opts = get_xoptions('-Xa', '-Xb=c,d=e')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have tests do X-options with values? Maybe we can use pycache_prefix to keep this covered.

Copy link
Member Author

Choose a reason for hiding this comment

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

test_embed has a bunch, but I can add one here

Copy link
Member Author

Choose a reason for hiding this comment

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

Corrected in the last commit! Thanks for the suggestion @felixxm

self.assertEqual(opts, {'a': True, 'b': 'c,d=e'})
opts = get_xoptions('-Xno_debug_ranges', '-Xdev=1234')
self.assertEqual(opts, {'no_debug_ranges': True, 'dev': '1234'})

@unittest.skipIf(interpreter_requires_environment(),
'Cannot run -E tests when PYTHON env vars are required.')
def test_unknown_xoptions(self):
rc, out, err = assert_python_failure('-X', 'blech')
self.assertIn(b'Unknown value for option -X', err)
msg = b'Fatal Python error: Unknown value for option -X'
self.assertEqual(err.splitlines().count(msg), 1)
self.assertEqual(b'', out)

def test_showrefcount(self):
def run_python(*args):
Expand Down
23 changes: 12 additions & 11 deletions Lib/test/test_embed.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ def test_pre_initialization_sys_options(self):
"test_pre_initialization_sys_options", env=env)
expected_output = (
"sys.warnoptions: ['once', 'module', 'default']\n"
"sys._xoptions: {'not_an_option': '1', 'also_not_an_option': '2'}\n"
"sys._xoptions: {'dev': '2', 'utf8': '1'}\n"
"warnings.filters[:3]: ['default', 'module', 'once']\n"
)
self.assertIn(expected_output, out)
Expand Down Expand Up @@ -822,15 +822,14 @@ def test_init_from_config(self):
'argv': ['-c', 'arg2'],
'orig_argv': ['python3',
'-W', 'cmdline_warnoption',
'-X', 'cmdline_xoption',
'-X', 'dev',
'-c', 'pass',
'arg2'],
'parse_argv': 2,
'xoptions': [
'config_xoption1=3',
'config_xoption2=',
'config_xoption3',
'cmdline_xoption',
'dev=3',
'utf8',
'dev',
],
'warnoptions': [
'cmdline_warnoption',
Expand Down Expand Up @@ -1048,9 +1047,8 @@ def test_init_sys_add(self):
config = {
'faulthandler': 1,
'xoptions': [
'config_xoption',
'cmdline_xoption',
'sysadd_xoption',
'dev',
'utf8',
'faulthandler',
],
'warnoptions': [
Expand All @@ -1060,9 +1058,12 @@ def test_init_sys_add(self):
],
'orig_argv': ['python3',
'-W', 'ignore:::cmdline_warnoption',
'-X', 'cmdline_xoption'],
'-X', 'utf8'],
}
self.check_all_configs("test_init_sys_add", config, api=API_PYTHON)
preconfig = {'utf8_mode': 1}
self.check_all_configs("test_init_sys_add", config,
expected_preconfig=preconfig,
api=API_PYTHON)

def test_init_run_main(self):
code = ('import _testinternalcapi, json; '
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Python now fails to initialize if it finds an invalid :option:`-X` option in the
command line. Patch by Pablo Galindo.
Copy link
Member

Choose a reason for hiding this comment

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

if... what? :-)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks for the fix :-)

18 changes: 8 additions & 10 deletions Programs/_testembed.c
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ static int test_pre_initialization_sys_options(void)
* relying on the caller to keep the passed in strings alive.
*/
const wchar_t *static_warnoption = L"once";
const wchar_t *static_xoption = L"also_not_an_option=2";
const wchar_t *static_xoption = L"utf8=1";
size_t warnoption_len = wcslen(static_warnoption);
size_t xoption_len = wcslen(static_xoption);
wchar_t *dynamic_once_warnoption = \
Expand All @@ -230,7 +230,7 @@ static int test_pre_initialization_sys_options(void)
PySys_AddWarnOption(L"module");
PySys_AddWarnOption(L"default");
_Py_EMBED_PREINIT_CHECK("Checking PySys_AddXOption\n");
PySys_AddXOption(L"not_an_option=1");
PySys_AddXOption(L"dev=2");
PySys_AddXOption(dynamic_xoption);

/* Delete the dynamic options early */
Expand Down Expand Up @@ -548,18 +548,17 @@ static int test_init_from_config(void)
L"-W",
L"cmdline_warnoption",
L"-X",
L"cmdline_xoption",
L"dev",
L"-c",
L"pass",
L"arg2",
};
config_set_argv(&config, Py_ARRAY_LENGTH(argv), argv);
config.parse_argv = 1;

wchar_t* xoptions[3] = {
L"config_xoption1=3",
L"config_xoption2=",
L"config_xoption3",
wchar_t* xoptions[2] = {
L"dev=3",
L"utf8",
};
config_set_wide_string_list(&config, &config.xoptions,
Py_ARRAY_LENGTH(xoptions), xoptions);
Expand Down Expand Up @@ -1375,7 +1374,6 @@ static int test_init_read_set(void)

static int test_init_sys_add(void)
{
PySys_AddXOption(L"sysadd_xoption");
PySys_AddXOption(L"faulthandler");
PySys_AddWarnOption(L"ignore:::sysadd_warnoption");

Expand All @@ -1387,14 +1385,14 @@ static int test_init_sys_add(void)
L"-W",
L"ignore:::cmdline_warnoption",
L"-X",
L"cmdline_xoption",
L"utf8",
};
config_set_argv(&config, Py_ARRAY_LENGTH(argv), argv);
config.parse_argv = 1;

PyStatus status;
status = PyWideStringList_Append(&config.xoptions,
L"config_xoption");
L"dev");
if (PyStatus_Exception(status)) {
goto fail;
}
Expand Down
48 changes: 48 additions & 0 deletions Python/initconfig.c
Original file line number Diff line number Diff line change
Expand Up @@ -2121,6 +2121,49 @@ _PyConfig_InitImportConfig(PyConfig *config)
return config_init_import(config, 1);
}

// List of known xoptions to validate against the provided ones. Note that all
// options are listed, even if they are only available if a specific macro is
// set, like -X showrefcount which requires a debug build. In this case unknown
// options are silently ignored.
const wchar_t* known_xoptions[] = {
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind to add a comment saying that all options are listed, even if they are only available if a specific macro is set, like -X showrefcount which requires a debug build (Py_REF_DEBUG). Say that unknown options are silently ignored in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

L"faulthandler",
L"showrefcount",
Copy link
Member

Choose a reason for hiding this comment

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

Should Python ignore silently this option if Python is not build with Py_REF_DEBUG? Or should we add #ifdef Py_REF_DEBUG? Using PYTHONDUMPREFS=1 env var doesn't fail with an error even if Python is not build with Py_TRACE_REFS.

IMO the least surprising behavior is to ignore silently the option. So leave the code as it is.

L"tracemalloc",
L"importtime",
L"dev",
L"utf8",
L"pycache_prefix",
L"warn_default_encoding",
L"no_debug_ranges",
L"frozen_modules",
NULL,
};

static const wchar_t*
_Py_check_xoptions(const PyWideStringList *xoptions, const wchar_t **names)
{
for (Py_ssize_t i=0; i < xoptions->length; i++) {
const wchar_t *option = xoptions->items[i];
size_t len;
wchar_t *sep = wcschr(option, L'=');
if (sep != NULL) {
len = (sep - option);
}
else {
len = wcslen(option);
}
int found = 0;
for (const wchar_t** name = names; *name != NULL; name++) {
if (wcsncmp(option, *name, len) == 0 && (*name)[len] == L'\0') {
found = 1;
}
}
if (found == 0) {
return option;
}
}
return NULL;
}

static PyStatus
config_read(PyConfig *config, int compute_path_config)
Expand All @@ -2136,6 +2179,11 @@ config_read(PyConfig *config, int compute_path_config)
}

/* -X options */
const wchar_t* option = _Py_check_xoptions(&config->xoptions, known_xoptions);
if (option != NULL) {
return PyStatus_Error("Unknown value for option -X");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe document in Doc/using/cmdline.rst that Python now fails if it gets an unknown -X option to help detecting typos.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you know if there is any way using the PyStatus_Error to provide a custom string? I would like to include the failed option but seems that nothing cleans the string provided here so I am not sure if is possible to customize the erro with the actual option :(

Copy link
Member

Choose a reason for hiding this comment

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

Do you know if there is any way using the PyStatus_Error to provide a custom string?

Currently, it's not possible. You should extend the API for that. You should add a static buffer just for that somewhere, or allocate a string, and decide when and how it's released. For PEP 587, I decided to keep it simple (KISS) and only use constant string.

}

if (config_get_xoption(config, L"showrefcount")) {
config->show_ref_count = 1;
}
Expand Down
3 changes: 3 additions & 0 deletions test_foo.py
2851
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
def foo(a=3, *, c, d=2):
pass
foo()
0