GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-31592: Fix an assertion failure in Python/ast.c in case of a bad unicodedata.normalize() by orenmn · Pull Request #3767 · python/cpython

Conversation

orenmn commented Sep 26, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

in ast.c: add a check whether unicodedata.normalize() returned a string.
in test_ast.py: add tests to verify that the assertion failure is no more.

https://bugs.python.org/issue31592

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          init commit

ff1fe99

the-knights-who-say-ni added the CLA signed label

Sep 26, 2017

bedevere-bot added the awaiting review label

Sep 26, 2017


          fix another bug in the C code

fb73e9f

orenmn commented Sep 27, 2017

Copy link

Copy Markdown

Contributor Author

Added another change to the patch, to fix the bug that Serhiy mentioned in https://bugs.python.org/issue31592#msg303043.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

orenmn added 2 commits

September 27, 2017 19:39


          use _PyList_ITEMS()

29f4604


          revert the change. of course, this is a tuple, not a list.

c29d5c0

serhiy-storchaka requested changes

Sep 28, 2017

View reviewed changes

Comment thread

Python/ast.c Outdated

		id2 = PyObject_Call(c->c_normalize, c->c_normalize_args, NULL);
		/* Use _PyObject_FastCall() this way to conceal c->c_normalize_args
		from the user. */
		id2 = _PyObject_FastCall(c->c_normalize,

serhiy-storchaka Sep 28, 2017

Copy link

Copy Markdown

Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

orenmn Sep 28, 2017

Copy link

Copy Markdown

Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 28, 2017

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 28, 2017

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

orenmn Sep 28, 2017

Copy link

Copy Markdown

Contributor Author

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

orenmn Sep 28, 2017

Copy link

Copy Markdown

Contributor Author

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 28, 2017

Copy link

Copy Markdown

Member

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.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

orenmn Sep 28, 2017

Copy link

Copy Markdown

Contributor Author

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.

cool, thanks for the explanation :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot commented Sep 28, 2017

Copy link

Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting changes and removed awaiting review labels

Sep 28, 2017


          completely remove c_normalize_args

cdaead9

orenmn commented Sep 28, 2017

Copy link

Copy Markdown

Contributor Author

I didn't expect the Spanish Inquisition!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting change review and removed awaiting changes labels

Sep 28, 2017

bedevere-bot commented Sep 28, 2017

Copy link

Copy Markdown

Nobody expects the Spanish Inquisition!

@serhiy-storchaka: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

serhiy-storchaka reviewed

Sep 28, 2017

View reviewed changes

Comment thread

Python/ast.c Outdated

		id2 = _PyObject_FastCall(c->c_normalize,
		((PyTupleObject *)c->c_normalize_args)->ob_item,
		2);
		PyObject *form = PyUnicode_FromString("NFKC");

serhiy-storchaka Sep 28, 2017

Copy link

Copy Markdown

Member

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.

Use _Py_IDENTIFIER().

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          use _Py_IDENTIFIER()

56c7ead

serhiy-storchaka approved these changes

Sep 28, 2017

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels

Sep 28, 2017


          Update 2017-09-26-16-05-04.bpo-31592.IFBZj9.rst

4e0a149

orenmn commented

Sep 30, 2017

View reviewed changes

Comment thread

Misc/NEWS.d/next/Core and Builtins/2017-09-26-16-05-04.bpo-31592.IFBZj9.rst

		@@ -1,2 +1,2 @@
		Fix an assertion failure in case of a bad `unicodedata.normalize()`. Patch
		by Oren Milman.
		Fixed an assertion failure in Python parser in case of a bad `unicodedata.normalize()`.

orenmn Sep 30, 2017

Copy link

Copy Markdown

Contributor Author

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.

Thanks for adding the 'Python parser' part :)

BTW, in https://devguide.python.org/committing/#what-s-new-and-news-entries, the example uses 'Fix ...' (as opposed to 'Fixed ...').
Which phrasing should be used? Or is it unimportant?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka Sep 30, 2017

Copy link

Copy Markdown

Member

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.

I prefer 'Fixed ...'. But my English is really bad. I just use old patterns.

About good and bad phrasing read this: https://mail.python.org/pipermail/python-dev/2011-May/111303.html.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

orenmn reacted with thumbs up emoji

All reactions

👍 1 reaction

serhiy-storchaka added needs backport to 3.6 type-bug An unexpected behavior, bug, or error labels

Sep 30, 2017

serhiy-storchaka merged commit 7dc46d8 into python:master

Sep 30, 2017

miss-islington commented Sep 30, 2017

Copy link

Copy Markdown

Contributor

Thanks @orenmn for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.6 label

Sep 30, 2017

bedevere-bot commented Sep 30, 2017

Copy link

Copy Markdown

GH-3836 is a backport of this pull request to the 3.6 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request

Sep 30, 2017


          [3.6] bpo-31592: Fix an assertion failure in Python parser in case of…

c97811b

… a bad unicodedata.normalize(). (pythonGH-3767)

(cherry picked from commit 7dc46d8)

serhiy-storchaka pushed a commit that referenced this pull request

Sep 30, 2017


          [3.6] bpo-31592: Fix an assertion failure in Python parser in case of…

a4dfe1c

… a bad unicodedata.normalize(). (GH-3767) (#3836)

(cherry picked from commit 7dc46d8)

Mariatta removed the awaiting merge label

Oct 8, 2017

Reviewers

serhiy-storchaka

serhiy-storchaka approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

type-bug

An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

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

Uh oh!

Uh oh!

orenmn commented Sep 27, 2017

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!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Sep 28, 2017

Uh oh!

orenmn commented Sep 28, 2017

Uh oh!

bedevere-bot commented Sep 28, 2017

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!

miss-islington commented Sep 30, 2017

Uh oh!

bedevere-bot commented Sep 30, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

orenmn commented Sep 26, 2017 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.