Repository navigation
gh-97556: Fix crash on null characters in string literal parser - #97577
apccurtiss wants to merge 6 commits into
Conversation
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
|
All commit authors signed the Contributor License Agreement. |
|
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
gvanrossum
changed the title
gvanrossum
left a comment
gvanrossum
left a comment
There was a problem hiding this comment.
This is a good catch, and the patch is almost perfect (I have one minor suggestion).
Could you also add a unittest that demonstrates the bug?
| if (len > INT_MAX) { | ||
| PyErr_SetString(PyExc_OverflowError, "string to parse is too long"); | ||
| return -1; |
There was a problem hiding this comment.
Maybe move this check to the top? I really don't care about the case where the string including quotes is just over INT_MAX but after stripping quotes it fits.
There was a problem hiding this comment.
Can do. Also, I just realized I copied the wrong issue number in the title (I meant #97556): is it enough to change the title, or should I create a whole new pull request?
apccurtiss
changed the title
|
I'm having trouble including a unit test due to a similar issue, #96670. Null characters raise errors when they are included in source code that is parsed using ast.parse, or which is imported from another file. So I have a test case for this which can be run directly, but it cannot be imported as part of the standard test suite. Any suggestions on how to work around that? |
You can write the test program to a file with a .py extension, manipulate sys.path so you can import that file, and attempt to import it. I think there are a few other tests that do that. E.g. look in test_bdb.py for import_helper. |
An easier way is to write the test code as a string and use the cpython/Lib/test/test_cmd_line_script.py Lines 220 to 226 in 68c46ae |
|
This PR is stale because it has been open for 30 days with no activity. |
| char *s; | ||
| Py_ssize_t len; | ||
|
|
||
| if (PyBytes_AsStringAndSize(t->bytes, &s, &len)) { |
There was a problem hiding this comment.
Shouldn't we reject this in the tokenizer instead? Otherwise files with NUL bytes in a string literal become valid source, while compile() still rejects NUL bytes.
| char *s; | ||
| Py_ssize_t len; | ||
|
|
||
| if (PyBytes_AsStringAndSize(t->bytes, &s, &len)) { |
There was a problem hiding this comment.
This is a crash fix but it lands without a regression test. I know a .py file with a null byte is blocked by gh-96670, but can we still add a regression test somehow, e.g. compiling a bytes source with an embedded null? We really want to keep this from regressing.
| @@ -0,0 +1 @@ | |||
| Fix interpreter crash when a string literal contains a null character. | |||
There was a problem hiding this comment.
Nit: this NEWS entry is filed under gh-issue-96670, but this PR fixes gh-97556. Can we rename it so the changelog points to the right issue?
github-actions
Bot
removed
the
stale
|
This PR is stale because it has been open for 90 days with no activity. |
4 participants
The _PyPegen_parsestr function was crashing on null characters inside of string literals due to a call to strlen, which treated it as a null terminator.
This change removes that call, which was actually redundant: the length was already calculated by the caller function and can be retrieved using PyBytes_AsStringAndSize. This has the added benefit of reducing the number of times the length gets calculated.