8000 bpo-31592: Fix an assertion failure in Python/ast.c in case of a bad unicodedata.normalize() by orenmn · Pull Request #3767 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

bpo-31592: Fix an assertion failure in Python/ast.c in case of a bad unicodedata.normalize() #3767

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.

Sign up for GitHub

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 7 commits into from
Sep 30, 2017
Merged

bpo-31592: Fix an assertion failure in Python/ast.c in case of a bad unicodedata.normalize() #3767

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ff1fe99
init commit
orenmn Sep 26, 2017
fb73e9f
fix another bug in the C code
orenmn Sep 27, 2017
29f4604
use _PyList_ITEMS()
orenmn Sep 27, 2017
c29d5c0
revert the change. of course, this is a tuple, not a list.
orenmn Sep 27, 2017
cdaead9
completely remove c_normalize_args
orenmn Sep 28, 2017
56c7ead
use _Py_IDENTIFIER()
orenmn Sep 28, 2017
4e0a149
Update 2017-09-26-16-05-04.bpo-31592.IFBZj9.rst
serhiy-storchaka Sep 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
revert the change. of course, this is a tuple, not a list.
  • Loading branch information
@orenmn
orenmn committed Sep 27, 2017
commit c29d5c011a55d4a1b74e0ddcf03d89e4f112e09b
3 changes: 2 additions & 1 deletion Python/ast.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,8 @@ new_identifier(const char *n, struct compiling *c)
/* Use _PyObject_FastCall() this way to conceal c->c_normalize_args
from the user. */
id2 = _PyObject_FastCall(c->c_normalize,
Copy link
Member

Choose a reason for hiding this comment

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

Don't use c->c_normalize_args. Use just a 2-element C array.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All right.
Would you mind to mention why this is better?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is with reusing the same tuple for passing arguments. If the fake normalize() save the reference to the tuple, it will see that an immutable tuple is mutated. It should be implemented in C, I can't reproduce the problem with Python code.

Just allocate a 2-element array on the stack, fill it with arguments, and pass it to the function. This will significantly simplify the code too.

Copy link
Member

Choose a reason for hiding this comment

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

Or just revert you last change, I'll create a separate PR. These bugs are related to the same function, but can be fixed separately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that we must conceal the tuple c->c_normalize_args from the user.
Doesn't my patch conceal it? I passed only the tuple items array to _PyObject_FastCall(), so even it doesn't have access to the tuple.
for example, _PyObject_FastCall() might eventually cause calling function_code_fastcall(), which would copy the args C array into f_localsplus, or it might call _PyStack_AsTuple(), which would copy the args C array into a new tuple.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or is my fix bad because it uses the internal structure of tuple?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, your patch conceal it. But using c->c_normalize_args as a buffer is suboptimal. You don't need a heap allocation, and the code would be simpler if use a stack variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, thanks for the explanation :)

_PyList_ITEMS(c->c_normalize_args), 2);
((PyTupleObject *)c->c_normalize_args)->ob_item,
2);
Py_DECREF(id);
if (!id2)
return NULL;
Expand Down
0