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

pablogsal · 2021-10-08T17:08:58Z

https://bugs.python.org/issue45445

pablogsal · 2021-10-08T17:10:03Z

Hey @vstinner, would you be ok merging some refined version of this draft? We found that using -X ---- can be very confusing and annoying when you make typos, so I think having errors if you pass some unknown stuff will be oughweighting any downside of making this more strict.

vstinner · 2021-10-11T09:15:32Z

Python/initconfig.c

@@ -2121,6 +2121,19 @@ _PyConfig_InitImportConfig(PyConfig *config)
    return config_init_import(config, 1);
 }

+const wchar_t* known_xoptions[] = {
+    L"faulthandler",
+    L"showrefcount",


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.

Python/preconfig.c

vstinner · 2021-10-11T09:18:35Z

Python/initconfig.c

@@ -2136,6 +2149,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");


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

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 :(

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.

pablogsal · 2021-10-12T13:02:06Z

@vstinner Done! I have addressed your feedback and I have updated some tests that were failing.

pablogsal · 2021-10-12T13:05:17Z

Lib/test/test_audit.py

@@ -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],


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

@vstinner @zooba

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

vstinner · 2021-10-12T14:42:10Z

Lib/test/test_audit.py

@@ -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],


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

vstinner · 2021-10-12T14:42:41Z

Lib/unittest/test/test_runner.py

+            pass
+
+        with self.assertRaises(TypeError):
+            foo(w=40000,z=50000,o=60000)


What is that?

Secrets 👀

vstinner · 2021-10-12T14:45:02Z

Python/initconfig.c

@@ -2121,6 +2121,45 @@ _PyConfig_InitImportConfig(PyConfig *config)
    return config_init_import(config, 1);
 }

+const wchar_t* known_xoptions[] = {


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.

vstinner · 2021-10-12T14:46:42Z

Python/initconfig.c

@@ -2136,6 +2149,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");


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.

…line

vstinner · 2021-10-12T14:58:05Z

Misc/NEWS.d/next/Core and Builtins/2021-10-12-14-41-39.bpo-45445._F5cMf.rst

@@ -0,0 +1,2 @@
+Python now fails to initialize if an invalid :option:`-X` option in the
+command line. Patch by Pablo Galindo.


if... what? :-)

Ah, thanks for the fix :-)

vstinner

LGTM. There is just a typo in the NEWS entry.

Misc/NEWS.d/next/Core and Builtins/2021-10-12-14-41-39.bpo-45445._F5cMf.rst

pablogsal · 2021-10-12T15:23:49Z

@vstinner I have added a small static buffer for now in b6fe156 to allow customizing the message. I will prepare a better generic API in a different PR in the future.

Python/initconfig.c

vstinner

LGTM.

felixxm

@pablogsal Thanks 👍 Looks great ⭐

Should we update an example in sys._xoptions docs? It crashes with this patch. We could use e.g. -Xutf8 -Xpycache_prefix=pycachepath.

felixxm · 2021-10-13T06:59:51Z

Lib/test/test_cmd_line.py

@@ -83,8 +83,17 @@ def get_xoptions(*args):
        opts = get_xoptions()
        self.assertEqual(opts, {})

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


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

test_embed has a bunch, but I can add one here

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

…bpo-45445._F5cMf.rst

…-41-39.bpo-45445._F5cMf.rst

felixxm

Thanks for updates 👍

tirkarthi · 2021-10-15T05:54:07Z

@pablogsal The PR has added test_foo.py in the repo's toplevel directory. Is it used somewhere in the tests where it's specifically needs to be in toplevel directory rather than under tests folders?

pablogsal · 2021-10-15T09:59:33Z

@pablogsal The PR has added test_foo.py in the repo's toplevel directory. Is it used somewhere in the tests where it's specifically needs to be in toplevel directory rather than under tests folders?

🤦‍♂️No, that was a mistake that slip us. I will clean it up. Apologies

pablogsal · 2021-10-15T10:04:16Z

🤦‍♂️No, that was a mistake that slip us. I will clean it up. Apologies

Fixed in #28972

tirkarthi · 2021-10-15T10:31:29Z

No worries, thanks @pablogsal :)

kovidgoyal · 2022-06-23T01:43:23Z

Cross posting:
Your documentation has stated for YEARS that arbitrary values can be passed via PyConfig.xoptions or -X. 9583cac

Quoting your own documentation:
"It also allows passing arbitrary values and retrieving them through the sys._xoptions dictionary."

There is client software out there relying on this. kovidgoyal/kitty#5223

Please do not randomnly break backwards compatibility, without even a deprecation period, because something "annoys" you.

Apparently in Python-land its acceptable behavior to break backward compatibility with documented interfaces on a whim. Bloody joke. python/cpyt F42D hon#28823 Fixes #5223

zooba · 2022-06-23T19:24:32Z

The documentation still says that arbitrary values may be passed: https://docs.python.org/3.11/using/cmdline.html#cmdoption-X

The -X command line argument is clearly public API and shouldn't change like this without a deprecation period. (I suspect in this case, adding a "this was possibly misspelled" warning is likely a better/less-hostile option than changing the behaviour anyway.)

sys._xoptions is in the weird place of being underscored and documented, and explicitly documented as being CPython specific, which would explain the leading underscore. But the definition of _xoptions isn't changing in this case - it's the change to -X making a previously documented and useful feature into an error, which requires deprecation.

gvanrossum · 2022-06-23T21:04:33Z

I've never had a use for sys._xoptions, but I've mistyped (or mis-guessed) -X flag names many times, so at least a warning about a misspelling would be appreciated.

I feel the python command line is not a public API in the same way that we talk about APIs for Python code. And doubly so for the -X flag which is (usually) used to control interpreter internals.

Before we roll this back I would like to understand what problem Kitty is solving by passing its own -X flags (the Kitty issue doesn't say, it was just immediately closed as "bug in CPython"). Maybe we can help the Kitty devs find a better solution for their problem?

arhadthedev · 2022-06-23T21:14:06Z

I feel the python command line is not a public API in the same way that we talk about APIs for Python code

I would say that CLI is an API for shell scripts. As a result, we need to account that eventhough scripts that pass nonexistent -W worked incorrectly, they worked without abruption or serious logical bug, and probably as a brick of some more complex production system.

gvanrossum · 2022-06-23T21:32:06Z

If a script was passing an incorrect -X option that was interpreted as a no-op, the new error just found a bug in the script -- presumably the script intended some other subtle effect that was never properly tested for.

pablogsal · 2022-06-23T21:39:02Z

Let's redirect the discussion to the issue (#89608) so we can discuss in a single place

kovidgoyal · 2022-06-24T03:55:50Z

On Thu, Jun 23, 2022 at 02:04:45PM -0700, Guido van Rossum wrote: I've never had a use for `sys._xoptions`, but I've mistyped (or mis-guessed) `-X` flag names many times, so at least a warning about a misspelling would be appreciated.

Yes, start with a warning, then after a couple of releases make it a fatal error. Starting with a fatal error is not the way.

I feel the python command line is not a public API in the same way that we talk about APIs for Python code. And doubly so for the `-X` flag which is (usually) used to control interpreter internals. Before we roll this back I would like to understand what problem Kitty is solving by passing its own `-X` flags (the Kitty issue doesn't say, it was just immediately closed as "bug in CPython"). Maybe we can help the Kitty devs find a better solution for their problem?

The problem was passing some data from the C runtime environment into the python interpreter. Before Python 3.8, there was no good way to run code between initialising the python interpreter and running it, without keeping track of a bunch of interpreter internals, that tended to change with versions. Since the introduction of PyConfig that is no longer the case. I now just initialise the interpreter and then set a dict on sys analogous to sys._xoptions myself using PySys_SetObject(). As I said, this issue does not affect me (or kitty) anymore. I reported it here because it seems very wrong to me to change documented API in the most hostile fashion possible (an interpreter fatal exit) without some kind of deprecation period or any consideration to backwards compat at all. And while here I will note that even now if someone sets up some scripts that run python as python -X foo and in the future foo is removed, it will again be a interpreter shutdown.

…mand line (pythonGH-28823)" This reverts commit db2b6a2.

… in the command line (GH-28823)" (#94745)

… in the command line (pythonGH-28823)" (pythonGH-94745) (cherry picked from commit aa37ffd) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

… in the command line (GH-28823)" (GH-94745) (cherry picked from commit aa37ffd) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>

bedevere-bot · 2022-07-31T16:41:13Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot s390x RHEL7 LTO 3.11 has failed when building commit 147a9a8.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/924/builds/291) and take a look at the build logs.
Check if the failure is related to this commit (147a9a8) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/924/builds/291

Summary of the results of the build (if available):

== Tests result: ENV CHANGED ==

415 tests OK.

10 slowest tests:

test_concurrent_futures: 2 min 45 sec
test_tools: 2 min 24 sec
test_multiprocessing_spawn: 2 min 22 sec
test_multiprocessing_forkserver: 1 min 24 sec
test_tokenize: 1 min 23 sec
test_multiprocessing_fork: 1 min 21 sec
test_asyncio: 1 min 15 sec
test_lib2to3: 1 min 2 sec
test_unparse: 59.4 sec
test_signal: 48.7 sec

1 test altered the execution environment:
test_asyncio

20 tests skipped:
test_devpoll test_gdb test_idle test_ioctl test_kqueue
test_launcher test_msilib test_smtpnet test_ssl test_startfile
test_tcl test_tix test_tk test_ttk_guionly test_ttk_textonly
test_turtle test_winconsoleio test_winreg test_winsound
test_zipfile64

Total duration: 6 min 37 sec

Click to see traceback logs

Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.11.edelsohn-rhel-z.lto/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
KeyError: '/psm_5e6812aa'


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.11.edelsohn-rhel-z.lto/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
KeyError: '/psm_ce63b861'


Traceback (most recent call last):
  File "/home/dje/cpython-buildarea/3.11.edelsohn-rhel-z.lto/build/Lib/multiprocessing/resource_tracker.py", line 209, in main
    cache[rtype].remove(name)
KeyError: '/psm_67b34c9e'

Fail if an invalid X-option is provided in the command line

4b7ea77

bedevere-bot added the awaiting core review label Oct 8, 2021

the-knights-who-say-ni added the CLA signed label Oct 8, 2021

pablogsal requested review from ericsnowcurrently, vstinner and gvanrossum October 8, 2021 17:10

vstinner reviewed Oct 11, 2021

View reviewed changes

pablogsal force-pushed the unknown branch from 5531b9e to 226c98d Compare October 12, 2021 13:03

pablogsal marked this pull request as ready for review October 12, 2021 13:03

pablogsal requested a review from vstinner October 12, 2021 13:03

pablogsal force-pushed the unknown branch from 226c98d to aa23ed2 Compare October 12, 2021 13:04

pablogsal commented Oct 12, 2021

View reviewed changes

pablogsal changed the title ~~Fail if an invalid X-option is provided in the command line~~ bpo-45445: Fail if an invalid X-option is provided in the command line Oct 12, 2021

pablogsal force-pushed the unknown branch from aa23ed2 to c40a383 Compare October 12, 2021 13:41

vstinner reviewed Oct 12, 2021

View reviewed changes

fixup! Fail if an invalid X-option is provided in the command line

33bf9dc

pablogsal force-pushed the unknown branch from c40a383 to 33bf9dc Compare October 12, 2021 14:50

fixup! fixup! Fail if an invalid X-option is provided in the command …

20dd756

…line

vstinner reviewed Oct 12, 2021

View reviewed changes

vstinner approved these changes Oct 12, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 12, 2021

pablogsal commented Oct 12, 2021

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2021-10-12-14-41-39.bpo-45445._F5cMf.rst Outdated Show resolved Hide resolved

pablogsal force-pushed the unknown branch from b85b1e6 to b6fe156 Compare October 12, 2021 15:23

pablogsal force-pushed the unknown branch from b6fe156 to 7796063 Compare October 12, 2021 17:01

vstinner reviewed Oct 12, 2021

View reviewed changes

Python/initconfig.c Outdated Show resolved Hide resolved

pablogsal force-pushed the unknown branch from 7796063 to 20dd756 Compare October 12, 2021 22:16

vstinner approved these changes Oct 12, 2021

View reviewed changes

felixxm reviewed Oct 13, 2021

View reviewed changes

pablogsal added 2 commits October 13, 2021 12:06

fixup! Update Misc/NEWS.d/next/Core and Builtins/2021-10-12-14-41-39.…

19598b5

…bpo-45445._F5cMf.rst

fixup! fixup! Update Misc/NEWS.d/next/Core and Builtins/2021-10-12-14…

dfd3e3e

…-41-39.bpo-45445._F5cMf.rst

felixxm approved these changes Oct 13, 2021

View reviewed changes

pablogsal merged commit db2b6a2 into python:main Oct 13, 2021

pablogsal deleted the unknown branch October 13, 2021 17:08

bedevere-bot removed the awaiting merge label Oct 13, 2021

merwok mentioned this pull request May 26, 2022

gh-90300: split --help output into separate options #30331

Merged

pablogsal mentioned this pull request Jun 23, 2022

Fail if an invalid -X option is provided #89608

Closed

ambv mentioned this pull request Jul 6, 2022

bpo-45445: Revert "bpo-45445: Remove incorrectly commited test file" #94587

Closed

pablogsal added a commit to pablogsal/cpython that referenced this pull request Jul 11, 2022

Revert "bpo-45445: Fail if an invalid X-option is provided in the com…

ec3a626

…mand line (pythonGH-28823)" This reverts commit db2b6a2.

pablogsal added a commit that referenced this pull request Jul 31, 2022

bpo-45445: Revert "bpo-45445: Fail if an invalid X-option is provided…

aa37ffd

… in the command line (GH-28823)" (#94745)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

		@@ -0,0 +1,2 @@
		Python now fails to initialize if an invalid :option:`-X` option in the
		command line. Patch by Pablo Galindo.

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

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

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️