gh-74616: Raise ValueError in case of NULL in input prompt #1738

kushaldas · 2017-05-22T23:51:24Z

If the input prompt to the builtin input function has any NULL
character, then raise ValueError.

Issue: input function truncates prompt by NULL byte #74616

eryksun · 2017-05-23T00:28:27Z

Python/bltinmodule.c

            promptstr = "";
        }
+        if (PyBytes_GET_SIZE(po) != strlen(promptstr)) {
+                PyErr_SetString(PyExc_ValueError,
+                                "input prompt has NULL character");


How about "input: prompt string cannot contain null characters"? "null" is more common in existing error messages, but "NUL" with one L is also used, since that's the ASCII name of character. NULL refers to the C pointer constant.

Sounds good, I will modify it for the same.

serhiy-storchaka · 2017-05-23T06:19:41Z

Doc/library/functions.rst

@@ -680,6 +680,15 @@ are always available.  They are listed here in alphabetical order.
   If the :mod:`readline` module was loaded, then :func:`input` will use it
   to provide elaborate line editing and history features.

+   .. versionchanged:: 3.7
+      If the input prompt string has a NULL character, then it will raise


NULL means a null pointer in C. Use "a null character" for consistency with other documentation.

But actually I think this change doesn't need special mentioning in the documentation. This is just a bugfix. A null character is prohibited in many other APIs.

Okay, will remove this docs update.

serhiy-storchaka · 2017-05-23T06:25:45Z

Python/bltinmodule.c

@@ -1976,9 +1976,15 @@ builtin_input_impl(PyObject *module, PyObject *prompt)
            promptstr = PyBytes_AS_STRING(po);
        }
        else {
-            po = NULL;
+            po = PyBytes_FromString("");


This can raise an error.

Yup, I will do a check for NULL case.

actually I don't need this anymore for the change as mentioned the comment below.

Python/bltinmodule.c

Lib/test/test_readline.py

If the input prompt to the builtin input function has any null character, then raise ValueError.

serhiy-storchaka · 2017-05-25T11:55:57Z

Lib/test/test_readline.py

@@ -127,6 +127,13 @@ def test_init(self):
 input()
 print("History length:", readline.get_current_history_length())
 """
+    auto_history_script_with_null = """\
+import readline
+readline.set_auto_history({})


Is readline history related to this issue? Does the test work if remove calls of set_auto_history and get_current_history_length?

I suspect the readline module is not needed at all. Perhaps the test would be better off in test_builtin.py. There are already some tests there in the PtyTests class that call input on a terminal, so you may be able to use them for inspiration.

Python/bltinmodule.c

terryjreedy

This change, if general, introduces a regression. The doc for input(prompt) says "If the prompt argument is present, it is written to standard output without a trailing newline." The handling of the prompt is up to stdout.write and thence physical displays devices. Python does not specify what happens with any particular character, including \r, \t, and astral chars. [This is an abbreviation of my 2017-05-27 posts on the issue]

brettcannon · 2018-02-02T22:04:56Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

github-actions · 2022-02-19T00:08:38Z

This PR is stale because it has been open for 30 days with no activity.

github-actions · 2022-08-28T00:15:30Z

This PR is stale because it has been open for 30 days with no activity.

iritkatriel · 2022-11-27T15:44:16Z

This change, if general, introduces a regression. The doc for input(prompt) says "If the prompt argument is present, it is written to standard output without a trailing newline." The handling of the prompt is up to stdout.write and thence physical displays devices. Python does not specify what happens with any particular character, including \r, \t, and astral chars. [This is an abbreviation of my 2017-05-27 posts on the issue]

@terryjreedy - are you saying that this PR should be rejected, or did I misunderstand?

Python/bltinmodule.c

terryjreedy · 2022-11-28T04:18:41Z

Yes, to me the patch is wrong. The doc still says "If the prompt argument is present, it is written to standard output without a trailing newline. " The issue OP, ghost, merely noted that the prompt was truncated. I believe 'ghost' wanted it to not be truncated. To me, raising instead of writing something is worse. It is not normal for Python to check strings for null. Print and sys.stdout.write and sys.stderr.write do not. From my testing of builtin functions, only the filename in open and compile is. The attr functions and int and iter do not.

Eryk says the prompt goes to stderr on terminals. If this is considered correct, then the doc should be changed.

eryksun · 2022-11-28T06:34:26Z

It is not normal for Python to check strings for null.

If a string is passed to C as a null-terminated string, ValueError should be raised if it contains embedded null characters. Allowing the string to get truncated arbitrarily is poorly defined behavior.

PyOS_Readline() in "Parser/myreadline.c" takes the prompt to write as a null-terminated string because that's what the GNU Readline API uses.

Print and sys.stdout.write and sys.stderr.write do not.

The underlying calls to POSIX write() and WinAPI WriteFile() and WriteConsoleW() require the number of data elements to write, so embedded null characters aren't a problem.

Eryk says the prompt goes to stderr on terminals. If this is considered correct, then the doc should be changed.

If sys.stdin and sys.stdout are terminal files and respectively the same OS files as the C stdin and stdout streams, then input() is implemented by PyOS_Readline().

The default implementation of PyOS_Readline() is PyOS_StdioReadline(), which writes the prompt to the C stderr stream¹. However, if the readline module is imported, then PyOS_Readline() typically uses GNU Readline on Unix systems. GNU Readline writes the prompt to stdout, not to stderr. Usually a script does not import the readline module.

You can examine the different behavior on a Unix system by executing a test script with the single line input('prompt: '). If you run it via python test.py 2>stderr.txt, the prompt will be written to "stderr.txt", and you won't have GNU Readline editing keys (e.g. home and end). If you run it via python -i test.py 2>stderr.txt, the prompt will be written to the terminal, and you'll have GNU Readline editing keys.

On Windows, if C stderr is a console file and legacy mode isn't enabled, we actually re-encode the prompt from UTF-8 to UTF-16 and write it directly to the console via WriteConsoleW(). ↩

terryjreedy · 2022-11-28T07:03:50Z

It would seem then that calling sys.stdout.write('prompt') (or sys.stderr? when appropriate?) and then PyOS_Readline(), with no prompt, would solve the truncation issue.

As for the destination, I am just suggesting adding 'or error' to the current doc to match the behavior.

eryksun · 2022-11-28T08:38:31Z

It would seem then that calling sys.stdout.write('prompt') (or sys.stderr? when appropriate?) and then PyOS_Readline(), with no prompt, would solve the truncation issue.

Unfortunately GNU Readline needs to be passed the prompt in order to set the margin for editing. Without it, for example, pressing the home key will move the cursor to the first column of the terminal instead of the first column after the prompt.

serhiy-storchaka · 2022-11-28T08:39:00Z

In Python interface to OS or other libraries an error is raised if the underlying function does not support embedded NULs (for example if it takes argument as a null-terminated string char*). In many cases the check is indirect (when use PyArg_Parse with the "s" format unit) or is embedded in other C API functions like PyUnicode_FSDecoder or PyUnicode_AsWideCharString, in other cases it is explicit.

And since the readline API accepts argument as null-terminated string, we should ensure that it does not contain embedded NULs.

Co-authored-by: Eryk Sun <[email protected]>

terryjreedy · 2022-11-28T17:30:22Z

Eryk's cursor control (prompt avoidance) issue, which IDLE has now, convinced me to drop my objection to the code change.

eryksun · 2022-11-28T19:57:45Z

@serhiy-storchaka, do you think PyRun_InteractiveOneObjectEx() in "Python/pythonrun.c" should raise a warning if sys.ps1 or sys.ps2 contain embedded null characters? I don't think raising an error would be appropriate for the REPL.

serhiy-storchaka · 2022-11-29T07:03:01Z

Good question. Since PyRun_InteractiveOneObjectEx() already silences many errors (like absent or non-UTF8-encodable sys.ps1), it would not be much worse if it silently truncate or completely ignore sys.ps1 with a null character. In future it would be better to emit some warnings or error, but it is a different issue, and it is not particularly important in any case.

github-actions · 2022-12-30T00:10:50Z

This PR is stale because it has been open for 30 days with no activity.

The objection was dropped.

…rompt (pythonGH-1738) If the input prompt to the builtin input function on terminal has any null character, then raise ValueError instead of silently truncating it. Co-authored-by: Serhiy Storchaka <[email protected]>

kushaldas requested a review from vadmium May 22, 2017 23:51

the-knights-who-say-ni added the CLA signed label May 22, 2017

eryksun reviewed May 23, 2017

View reviewed changes

kushaldas force-pushed the fix-issue-30431 branch from 6a47483 to b0b45fd Compare May 23, 2017 01:03

serhiy-storchaka requested changes May 23, 2017

View reviewed changes

kushaldas force-pushed the fix-issue-30431 branch from b0b45fd to fa1a030 Compare May 23, 2017 17:48

bpo-30431: Raise ValueError in case of null in input prompt

72c5af2

If the input prompt to the builtin input function has any null character, then raise ValueError.

kushaldas force-pushed the fix-issue-30431 branch from fa1a030 to 72c5af2 Compare May 24, 2017 21:09

serhiy-storchaka reviewed May 25, 2017

View reviewed changes

terryjreedy previously requested changes May 27, 2017

View reviewed changes

brettcannon added the awaiting changes label Feb 2, 2018

github-actions bot added the stale Stale PR or inactive for long period of time. label Feb 19, 2022

erlend-aasland changed the title ~~bpo-30431: Raise ValueError in case of NULL in input prompt~~ gh-74616: Raise ValueError in case of NULL in input prompt Jun 29, 2022

ezio-melotti removed the CLA signed label Jul 13, 2022

github-actions bot removed the stale Stale PR or inactive for long period of time. label Jul 28, 2022

github-actions bot added the stale Stale PR or inactive for long period of time. label Aug 28, 2022

Merge branch 'main' into fix-issue-30431

de47b91

eryksun reviewed Nov 27, 2022

View reviewed changes

Python/bltinmodule.c Outdated Show resolved Hide resolved

github-actions bot removed the stale Stale PR or inactive for long period of time. label Nov 28, 2022

Update Python/bltinmodule.c

1b2b47b

Co-authored-by: Eryk Sun <[email protected]>

github-actions bot added the stale Stale PR or inactive for long period of time. label Dec 30, 2022

serhiy-storchaka added 2 commits December 6, 2023 17:14

Merge branch 'main' into fix-issue-30431

5fabfca

Rewrite the test and add few more tests.

25d8b60

serhiy-storchaka approved these changes Dec 6, 2023

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting changes labels Dec 6, 2023

github-actions bot removed the stale Stale PR or inactive for long period of time. label Dec 7, 2023

serhiy-storchaka enabled auto-merge (squash) December 7, 2023 09:57

Add a NEWS entry.

d08bc57

serhiy-storchaka merged commit 4ba15de into python:main Dec 7, 2023

bedevere-app bot removed the awaiting merge label Dec 7, 2023

ghost mentioned this pull request Apr 10, 2022

input function truncates prompt by NULL byte #74616

Closed

Uh oh!

gh-74616: Raise ValueError in case of NULL in input prompt #1738

gh-74616: Raise ValueError in case of NULL in input prompt #1738

Uh oh!

Conversation

kushaldas commented May 22, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vadmium May 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

terryjreedy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brettcannon commented Feb 2, 2018

Uh oh!

github-actions bot commented Feb 19, 2022

Uh oh!

github-actions bot commented Aug 28, 2022

Uh oh!

iritkatriel commented Nov 27, 2022

Uh oh!

Uh oh!

terryjreedy commented Nov 28, 2022

Uh oh!

eryksun commented Nov 28, 2022

Footnotes

Uh oh!

terryjreedy commented Nov 28, 2022

Uh oh!

eryksun commented Nov 28, 2022

Uh oh!

serhiy-storchaka commented Nov 28, 2022

Uh oh!

terryjreedy commented Nov 28, 2022

Uh oh!

eryksun commented Nov 28, 2022

Uh oh!

serhiy-storchaka commented Nov 29, 2022

Uh oh!

github-actions bot commented Dec 30, 2022

Uh oh!

Uh oh!

kushaldas commented May 22, 2017 •

edited by bedevere-bot

Loading

vadmium May 27, 2017 •

edited

Loading

terryjreedy left a comment •

edited

Loading