GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-40661: Fix segfault when parsing invalid input by lysnikolaou · Pull Request #20165 · python/cpython

Conversation

@lysnikolaou

@lysnikolaou lysnikolaou commented May 17, 2020 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Fix segfaults when parsing very complex invalid input, like
import äˆ ð£„¯ð¢·žð±‹á”€ð””ð‘©±å®ä±¬ð©¾\n𗶽.

Initially reported by @pablogsal.

https://bugs.python.org/issue40661

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lysnikolaou
bpo-40661: Fix segfault when parsing invalid input
a1a9d28 
Fix segfaults when parsing very complex invalid input, like
`import äˆ ð£„¯ð¢·žð±‹á”€ð””ð‘©±å®ä±¬ð©¾\n𗶽`.

Initially reported by @pablogsal.
@lysnikolaou
lysnikolaou requested a review from pablogsal as a code owner May 17, 2020 23:11
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 17, 2020
@bedevere-bot bedevere-bot added the awaiting review label May 17, 2020
@lysnikolaou lysnikolaou added the skip news label May 17, 2020
@lysnikolaou
Check for errors at the beginning of each rule as well
9ec03ba 
This is not really needed, but it's nice to have, so that we don't
unnecessariy call `_PyPegen_fill_token` when there are errors and
the parser will abort anyway.
aeros
aeros reviewed May 18, 2020
View reviewed changes

@aeros aeros left a comment

Copy link
Copy Markdown
Contributor

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 was experimenting with the reproducer locally and might have discovered a more minimalist solution. I'm not certain as to whether this approach is any better or worse (my experience and knowledge w/ the parser is very limited), but this also seems to address the segfault:

diff --git a/Lib/test/test_peg_parser.py b/Lib/test/test_peg_parser.py
index 9614e45799..05de8582e7 100644
--- a/Lib/test/test_peg_parser.py
+++ b/Lib/test/test_peg_parser.py
@@ -591,6 +591,7 @@ FAIL_TEST_CASES = [
     ("f-string_single_closing_brace", "f'}'"),
     ("from_import_invalid", "from import import a"),
     ("from_import_trailing_comma", "from a import b,"),
+    ("import_very_complex", "import äˆ ð£„¯ð¢·žð±‹á”€ð””ð‘©±å®ä±¬ð©¾\n𗶽"),
     # This test case checks error paths involving tokens with uninitialized
     # values of col_offset and end_col_offset.
     ("invalid indentation",
diff --git a/Parser/pegen/pegen.c b/Parser/pegen/pegen.c
index 7f3e4561de..25b8db0982 100644
--- a/Parser/pegen/pegen.c
+++ b/Parser/pegen/pegen.c
@@ -758,6 +758,9 @@ _PyPegen_lookahead(int positive, void *(func)(Parser *), Parser *p)
 Token *
 _PyPegen_expect_token(Parser *p, int type)
 {
+    if (p->error_indicator) {
+        return NULL;
+    }
     if (p->mark == p->fill) {

Would it make any sense to check the error indicator at the beginning of _PyPegen_expect_token()?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@lysnikolaou

lysnikolaou commented May 18, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

IMHO the solution Pablo found might be a bit more verbose and add 3 lines of code per alternative, but it makes more sense and is something we should have done, when we initially replaced setjmp with p->error_indicator. We generally want the parser to abort as quickly as possible, when there's an error, and that includes not going through the other alternatives of the rule that fails and all those above it in call tree.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum
gvanrossum approved these changes May 18, 2020
View reviewed changes
Comment thread Lib/test/test_peg_parser.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels May 18, 2020
@gvanrossum
Update Lib/test/test_peg_parser.py
b7be72c
@gvanrossum

gvanrossum commented May 18, 2020

Copy link
Copy Markdown
Member

@aeros What would really be helpful here is to understand the sequence of events that happens when the parser encounters that input, and compare it to what happens on similar ASCII input like import a ?. What path (presumably involving the tokenizer) is taken differently?
All reactions

Sorry, something went wrong.

Uh oh!

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

pablogsal commented May 18, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

What path (presumably involving the tokenizer) is taken differently?

The difference is that when the import is not ascii, this code path is taken:

https://github.com/python/cpython/blob/master/Parser/pegen/pegen.c#L90-L105

and the call to id2 = _PyObject_FastCall(p->normalize, args, 2); is the one that fails because when we enter that call, we already have an exception set from tokenize_error, which is the exception that should be reported.

So basically: for ASCII characters there is one call less that makes the error surface without problems, but with non-ASCII characters, there is some call into Python code that fails because is done with an exception already set.

This can happen in so many other ways, so I think this PR is the safest thing to do because we make sure that nothing more is tried (and therefore no other calls can occur) when the error indicator is set.
aeros reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pablogsal
pablogsal reviewed May 18, 2020
View reviewed changes
Comment thread Lib/test/test_peg_parser.py
("f-string_single_closing_brace", "f'}'"),
("from_import_invalid", "from import import a"),
("from_import_trailing_comma", "from a import b,"),
("import_non_ascii_syntax_error", "import ä £"),

@pablogsal pablogsal May 18, 2020

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.

Could you add this to test_syntax as well? This file will likely go away when we don't have two parsers to compare against anymore.

Sorry, something went wrong.

Uh oh!

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

@pablogsal pablogsal May 18, 2020

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 have pushed this myself in commit 7353b6c

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
lysnikolaou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@pablogsal
Add test also to test_syntax
7353b6c
pablogsal
pablogsal approved these changes May 18, 2020
View reviewed changes
@pablogsal
pablogsal merged commit 7b7a21b into python:master May 18, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label May 18, 2020
@lysnikolaou
lysnikolaou deleted the fix-segfault branch May 18, 2020 17:33
arturoescaip pushed a commit to arturoescaip/cpython that referenced this pull request May 24, 2020
@lysnikolaou @gvanrossum
@pablogsal @arturoescaip
bpo-40661: Fix segfault when parsing invalid input (pythonGH-20165)
2d6bc90 
Fix segfaults when parsing very complex invalid input, like `import äˆ ð£„¯ð¢·žð±‹á”€ð””ð‘©±å®ä±¬ð©¾\n𗶽`.

Co-authored-by: Guido van Rossum <[email protected]>
Co-authored-by: Pablo Galindo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@pablogsal pablogsal pablogsal approved these changes
+1 more reviewer
@aeros aeros aeros left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

skip news

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

@lysnikolaou @gvanrossum @pablogsal @aeros @the-knights-who-say-ni @bedevere-bot
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.