Fix crash on malformed nested brace/paren patterns - #68
Open
afonsojanu wants to merge 1 commit into
Open
Conversation
expand() throws "Cannot read properties of undefined (reading 'push')"
on patterns like {{*(() instead of falling back to the literal string
the way other malformed brace patterns already do.
walk() climbs a node's parent chain looking for the nearest ancestor
typed 'brace' or 'root' so it can reuse that ancestor's queue array.
That climb assumes every such ancestor already got its queue
initialized, which normally happens because walk() sets node.queue = []
the moment it's called on a node. For this pattern though, the parser
builds a paren node whose parent chain includes a brace node that
walk() never actually visits directly, since the parser leaves it out
of the tree that gets walked for input this broken. Its queue stays
undefined, and the next push on it blows up.
Falling back to an empty array when the climb lands on a queue-less
node keeps the same "give back the input mostly as-is" behavior this
file already uses elsewhere for other kinds of malformed patterns,
instead of crashing.
Added a regression test in test/regression.js covering the reported
pattern plus two related malformed shapes, and confirmed it fails
without the fix and passes with it. Full suite (895 tests) stays
green.
This file contains hidden or 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
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.
Fixes #60.
The crash is in
walk(). When it hits a leaf node with a value, it climbs the node's parent chain looking for the nearest ancestor typedbraceorroot, then pushes onto that ancestor'squeuearray:That assumes the ancestor it lands on already has
queueset, which normally happens becausewalk()setsnode.queue = []right when it's called on a node. For this particular pattern, the parser builds aparennode whose parent chain runs through abracenode thatwalk()never actually visits on its own, since the parser leaves it out of the tree it hands back for input this malformed. So that ancestor'squeuestaysundefined, and pushing onto it blows up.I traced this with some throwaway logging rather than guessing: for
{{*((), the crash happens exactly when reaching the innerparennode's()value, at which pointblock.typeis'brace'andblock.queueisundefined.The fix just falls back to an empty array in both places this climb happens, when it lands on a node without a queue. This matches how the rest of this function already treats other malformed patterns (falling back to something reasonable rather than throwing), so
{{*(()now expands to['{{*(']instead of crashing.Added a test in
test/regression.jsalongside the existing malformed-pattern cases there, covering the reported pattern plus two related shapes ({{{and((()). Confirmed it fails on master and passes with the fix. Ran the full suite locally, 895 passing.