The new parser segfaults when parsing invalid input

[pablogsal]

BPO	40661
Nosy	@gvanrossum, @vstinner, @lysnikolaou, @Zac-HD, @pablogsal
PRs	bpo-40661: Fix segfaults when parsing invalid input #20159 bpo-40661: Fix segfault when parsing invalid input #20165
Files	reproducer.py

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2020-05-18.17:32:26.774>
created_at = <Date 2020-05-17.16:01:10.114>
labels = ['interpreter-core', 'deferred-blocker', '3.9', 'type-crash']
title = 'The new parser segfaults when parsing invalid input'
updated_at = <Date 2020-05-18.17:32:26.773>
user = 'https://github.com/pablogsal'

bugs.python.org fields:

activity = <Date 2020-05-18.17:32:26.773>
actor = 'pablogsal'
assignee = 'none'
closed = True
closed_date = <Date 2020-05-18.17:32:26.774>
closer = 'pablogsal'
components = ['Interpreter Core']
creation = <Date 2020-05-17.16:01:10.114>
creator = 'pablogsal'
dependencies = []
files = ['49169']
hgrepos = []
issue_num = 40661
keywords = ['patch']
message_count = 13.0
messages = ['369132', '369133', '369134', '369136', '369142', '369144', '369164', '369176', '369181', '369235', '369249', '369263', '369280']
nosy_count = 5.0
nosy_names = ['gvanrossum', 'vstinner', 'lys.nikolaou', 'Zac Hatfield-Dodds', 'pablogsal']
pr_nums = ['20159', '20165']
priority = 'deferred blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'crash'
url = 'https://bugs.python.org/issue40661'
versions = ['Python 3.9']

[pablogsal]

The new peg parser segfaults when parsing the attached reproducer.

this is due to the fact that the exception set by tokenizer_error is not properly propagated.

[pablogsal]

I think we may need to test for the error indicator (and maybe PyErr_Ocurred for safety) before every alternative. Something like:

diff --git a/Tools/peg_generator/pegen/c_generator.py b/Tools/peg_generator/pegen/c_generator.py
index 8f9972bb41..61cb694628 100644
--- a/Tools/peg_generator/pegen/c_generator.py
+++ b/Tools/peg_generator/pegen/c_generator.py
@@ -468,10 +468,6 @@ class CParserGenerator(ParserGenerator, GrammarVisitor):
memoize = self._should_memoize(node)

         with self.indent():
-            self.print("if (p->error_indicator) {")
-            with self.indent():
-                self.print("return NULL;")
-            self.print("}")
             self.print(f"{result_type} _res = NULL;")
             if memoize:
                 self.print(f"if (_PyPegen_is_memoized(p, {node.name}_type, &_res))")
@@ -685,6 +681,12 @@ class CParserGenerator(ParserGenerator, GrammarVisitor):
     def visit_Alt(
         self, node: Alt, is_loop: bool, is_gather: bool, rulename: Optional[str]
     ) -> None:
+        self.print("if (p->error_indicator == 1 || PyErr_Occurred()) {")
+        with self.indent():
+            self.print("p->error_indicator = 1;")
+            self.print("return NULL;")
+        self.print("}")
+
         self.print(f"{{ // {node}")
         with self.indent():
             # Prepare variable declarations for the alternative

[pablogsal]

Indeed, that diff solves the problem

[gvanrossum]

How costly is PyErr_Occurred()? That worries me most, otherwise I’d accept this right away.

[pablogsal]

A quick benchmark using xxl.py:

Base time (master):
Time: 9.386 seconds on an average of 20 runs

With the patch in this issue:
Time: 9.498 seconds on an average of 20 runs

Sadly I could not test with PGO/LTO and without CPU isolation, so it would be great if someone could double-check these numbers.

Also, I will be unable to do a PR until this night/tomorrow morning (London time) :(

[gvanrossum]

I see almost no time difference for 'make time_stdlib': before 3.471, after 3.451. But I see a serious difference for 'make time_compile': before 3.474, after 4.996. That's over 40% slower (on the extreme case, xxl.py).

I'll prepare a PR just in case.

[Zac-HD]

I understand from Paul Ganssle that this bug was found using Hypothesmith in my stdlib property tests (reported at Zac-HD/stdlib-property-tests#14).

As discussed in we-like-parsers#91 and https://pyfound.blogspot.com/2020/05/property-based-testing-for-python.html I'm keen to help out how I can, so if there's anything more specific than "write tools, write test, and wait" please let me know!

Best,
Zac