-
-
Notifications
You must be signed in to change notification settings - Fork 32k
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
Changes from all commits
4b7ea77
33bf9dc
20dd756
ceedd70
19598b5
dfd3e3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,8 +83,17 @@ def get_xoptions(*args): | |
opts = get_xoptions() | ||
self.assertEqual(opts, {}) | ||
|
||
opts = get_xoptions('-Xa', '-Xb=c,d=e') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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): | ||
|
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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if... what? :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, thanks for the fix :-) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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[] = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! |
||
L"faulthandler", | ||
L"showrefcount", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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) | ||
|
@@ -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"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you know if there is any way using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
2851 | def foo(a=3, *, c, d=2): | |
pass | ||
foo() |
There was a problem hiding this comment.
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 notutf8
so it was silently ignored :(@vstinner @zooba
There was a problem hiding this comment.
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.