fix(parser): loop instead of recurse to avoid stack overflow#38
Open
zzorba wants to merge 1 commit intoEneris:masterfrom
Open
fix(parser): loop instead of recurse to avoid stack overflow#38zzorba wants to merge 1 commit intoEneris:masterfrom
zzorba wants to merge 1 commit intoEneris:masterfrom
Conversation
Owner
|
Can you please remove the dist folder from your pull? |
The MCS parser drove its state machine via mutual recursion between incomplete varint header would re-enter #waitForData with an unchanged buffer (the length check still passed for kSizePacketLenMin = 1), so a burst of queued messages or a TCP segment that split a varint size header could throw "RangeError: Maximum call stack size exceeded" out of the socket data handler. Convert #waitForData into a `while (!#destroyed && !#isWaitingForData)` loop. Handlers update #state / #data and return; they no longer call payload" branches set #isWaitingForData = true and bail so the loop won't tight-spin on a buffer that needs more bytes, and a #destroyed flag lets #emitError unwind cleanly. Verified against the patched parser: - 50,001 zero-byte HeartbeatPing frames in a single buffer parse without throwing (would have overflowed the stack previously). - A varint size header split across three TCP reads (0x80, then 0x01, then payload) parses one complete message with no busy loop. Also commit dist/ on this branch (and drop dist from .gitignore + remove prepare/prepublish scripts) so consumers installing from this git URL get a prebuilt package without needing to run the protobufjs codegen + tsc build at install time. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
zzorba
force-pushed
the
filament/loop-parser
branch
from
dcb0261 to
a424caa
Compare
Author
|
Sorry about that, I hadn't intended to upstream this but I guess now that its here it's worth it. We were using this library and received some stack traces that looked like an infinite-stack-recursion crash from this library. Claude seemed to think there was a corner case where a long running process would keep retrying via recursion -- this changes it to use a loop instead. |
Comment on lines
+111
to
+123
| this.#handleGotVersion() | ||
| this.#handleGotMessageTag() | ||
| this.#handleGotMessageSize() | ||
| break | ||
| case ProcessingState.MCS_TAG_AND_SIZE: | ||
| this.#handleGotMessageTag() | ||
| this.#handleGotMessageSize() | ||
| break | ||
| case ProcessingState.MCS_SIZE: | ||
| this.#handleGotMessageSize() | ||
| break | ||
| case ProcessingState.MCS_PROTO_BYTES: | ||
| this.#handleGotMessageBytes() |
Owner
There was a problem hiding this comment.
Copilot proposed change sounds a bit excessive, but it has a point. Can you check it out for the PR please? @zzorba
Owner
|
Othervise the PR looks good. I'll do some testing and let you know |
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.
The MCS parser drove its state machine via mutual recursion between #waitForData, #handleGotMessageSize, #handleGotMessageBytes, and #getNextMessage. Each buffered frame added two stack frames, and an incomplete varint header would re-enter #waitForData with an unchanged buffer (the length check still passed for kSizePacketLenMin = 1), so a burst of queued messages or a TCP segment that split a varint size header could throw "RangeError: Maximum call stack size exceeded" out of the socket data handler.
Convert #waitForData into a
while (!#destroyed && !#isWaitingForData)loop. Handlers update #state / #data and return; they no longer call #waitForData() directly. The "incomplete varint" and "incomplete payload" branches set #isWaitingForData = true and bail so the loop won't tight-spin on a buffer that needs more bytes, and a #destroyed flag lets #emitError unwind cleanly.Verified against the patched parser: