GeezerVison is showing github.com — The new parser segfaults when parsing invalid input · Issue #84838 · python/cpython
The new parser segfaults when parsing invalid input #84838Description
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'] Activitypablogsal commented on May 17, 2020 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 commented on May 17, 2020 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 alternativepablogsal commented on May 17, 2020 Indeed, that diff solves the problem gvanrossum commented on May 17, 2020 How costly is PyErr_Occurred()? That worries me most, otherwise I’d accept this right away. pablogsal commented on May 17, 2020 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 commented on May 17, 2020 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 commented on May 18, 2020 gvanrossum commented on May 18, 2020 Zac: The reproducer here apparently uses a long string of weird accented characters. I'm not sure how to generalize from that to other things that Hyothes* could find. But maybe this helps: #20106 (comment) Zac-HD commented on May 18, 2020 I know what else it might find either, but I still think it's worth running property-based tests in CI to find out! The demo I wrote for my language summit talk doesn't have any parser tests, but still would have caught this bug in the pull request that introduced it.
The specific reproducer here is odd, because it's reported as an internal error in Hypothesmith - I use the compile() builtin to check that strings valid against an approximate grammar are actually valid.
It's structurally less complex than typical outputs because it's only a fragment of the tree being generated, but because shrinking doesn't run for generation-time errors it's also much harder to interpret than usual. gvanrossum commented on May 18, 2020 Unfortunately, I do not understand enough about how Hypothes* works to be helpful here, other than by offering encouragement. (And please don't try to educate me. I have too many other things. Sorry.) vstinner commented on May 18, 2020 I don't think that such bug should block Python 3.9 beta1 (I'm talking about the "release blocker" priority). There is still time before 3.9 final to fix it. gvanrossum commented on May 18, 2020 Okay, deferring the blocker. But I'll still land Lysandros' fix ASAP. pablogsal commented on May 18, 2020
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|