Skip to content

The new parser segfaults when parsing invalid input #84838

Closed
Closed
The new parser segfaults when parsing invalid input#84838
Labels
3.9 (EOL)end of lifedeferred-blockerinterpreter-core(Objects, Python, Grammar, and Parser dirs)type-crashA hard crash of the interpreter, possibly with a core dump

Description

@pablogsal
pablogsal
opened on May 17, 2020
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']

    Activity

    1. pablogsal commented on May 17, 2020

      @pablogsal
      pablogsal
      on May 17, 2020
      MemberAuthor

      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.

    2. pablogsal
      added
      release-blocker
      3.9 (EOL)end of life
      interpreter-core(Objects, Python, Grammar, and Parser dirs)
      type-crashA hard crash of the interpreter, possibly with a core dump
      on May 17, 2020

    3. pablogsal commented on May 17, 2020

      @pablogsal
      pablogsal
      on May 17, 2020
      MemberAuthor

      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

    4. pablogsal commented on May 17, 2020

      @pablogsal
      pablogsal
      on May 17, 2020
      MemberAuthor

      Indeed, that diff solves the problem

    5. gvanrossum commented on May 17, 2020

      @gvanrossum
      gvanrossum
      on May 17, 2020
      Member

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

    6. pablogsal commented on May 17, 2020

      @pablogsal
      pablogsal
      on May 17, 2020
      MemberAuthor

      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) :(

    7. gvanrossum commented on May 17, 2020

      @gvanrossum
      gvanrossum
      on May 17, 2020
      Member

      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.

    8. Zac-HD commented on May 18, 2020

      @Zac-HD
      Zac-HDmannequin
      on May 18, 2020
      Mannequin

      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

    9. gvanrossum commented on May 18, 2020

      @gvanrossum
      gvanrossum
      on May 18, 2020
      Member

      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)

    10. Zac-HD commented on May 18, 2020

      @Zac-HD
      Zac-HDmannequin
      on May 18, 2020
      Mannequin

      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.

    11. gvanrossum commented on May 18, 2020

      @gvanrossum
      gvanrossum
      on May 18, 2020
      Member

      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.)

    12. vstinner commented on May 18, 2020

      @vstinner
      vstinner
      on May 18, 2020
      Member

      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.

    13. gvanrossum commented on May 18, 2020

      @gvanrossum
      gvanrossum
      on May 18, 2020
      Member

      Okay, deferring the blocker. But I'll still land Lysandros' fix ASAP.

    14. gvanrossum
      added
      deferred-blocker
      and removed
      release-blocker
      on May 18, 2020

    15. pablogsal commented on May 18, 2020

      @pablogsal
      pablogsal
      on May 18, 2020
      MemberAuthor

      New changeset 7b7a21b by Lysandros Nikolaou in branch 'master':
      bpo-40661: Fix segfault when parsing invalid input (GH-20165)
      7b7a21b

    16. pablogsal
      closed this as completedon May 18, 2020
    17. pablogsal
      closed this as completedon May 18, 2020
    18. ezio-melotti
      transferred this issue fromon Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Metadata

    Metadata

    Assignees

    No one assigned

      Labels

      3.9 (EOL)end of lifedeferred-blockerinterpreter-core(Objects, Python, Grammar, and Parser dirs)type-crashA hard crash of the interpreter, possibly with a core dump

      Projects

      No projects

        Milestone

        No milestone

        Relationships

        None yet

        Development

        No branches or pull requests

        Issue actions