Postpone position assignment #105
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This test case exposes a bug which causes the upper limit of a finite bound to be ignored in certain cases. The regular expression used in the test case is a minimal reproducer; a more practical but less readable and less easily debugged example would be a regular expression that accepts fully-qualified domain names.
During AST expansion, it is possible for new literal nodes to be created and given the same position as existing literals. The code attempts to reposition nodes when this happens but does not always succeed. The result is a short-circuit in the TNFA which can manifest as an infinite loop, e.g. a bounded iteration being treated as an unbounded one. This reshuffling can also leave positions unfilled, wasting memory. Instead of trying to assign positions to literal nodes on creation, and then having to renumber them on the fly during AST expansion, wait until the end and move position assignment into `tre_compute_nfl()`, which we rename to `tre_compute_npfl()`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
During AST expansion, it is possible for new literal nodes to be created and given the same position as existing literals. The code attempts to reposition nodes when this happens but does not always succeed. The result is a short-circuit in the TNFA which can manifest as an infinite loop, e.g. a bounded iteration being treated as an unbounded one. This reshuffling can also leave positions unfilled, wasting memory.
This bug was discovered while investigating a report of an issue with a regular expression intended to accept DNS names, which turned out to accept labels of any length, instead of only up to 63 characters. Compiling the minimal reproducer
^((a{1,3})?x)*ywith debugging enabled showed the following AST after expansion:Note that the last
anode and thexnode both have position 1. Theynode was moved from position 2 to position 4 during expansion; thexnode should have been moved to position 3, but wasn't.The solution is to postpone assigning positions to literal nodes until immediately before TNFA conversion, removing the need to reposition nodes during AST expansion. With this change, we get the following AST instead:
This PR adds test cases which trigger the bug and implements the solution described above.