fix: recover from parser errors instead of crashing the process#2138
Open
GiHoon1123 wants to merge 1 commit into
Open
fix: recover from parser errors instead of crashing the process#2138GiHoon1123 wants to merge 1 commit into
GiHoon1123 wants to merge 1 commit into
Conversation
redis-parser recurses once per level of RESP array nesting with no depth limit, so a sufficiently deeply nested reply throws a synchronous RangeError instead of a reportable parser error. Because DataHandler's stream "data" listener had no try/catch around parser.execute(), this RangeError escaped as an uncaught exception, crashing the process. Wrap parser.execute() in try/catch and route failures through the existing returnFatalError -> recoverFromFatalError recovery path, which already handles parser-detected fatal errors the same way: flush the in-flight command queue, keep the not-yet-sent offline queue intact, and reconnect. Fixes redis#2108
Contributor
|
@GiHoon1123 thank you for this PR, I'll take a look, however please check #2127 as it might make sense to adapt the change for that PR. |
Author
|
Thanks for the pointer! I took a look at #2127 — its Since #2127 is still in progress, this PR still seemed worth having as a fix for the currently released behavior - |
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.
Summary
redis-parserrecursively parses RESP arrays without a depth limit.A deeply nested reply can overflow the JS call stack and throw a synchronous
RangeError.DataHandlercurrently callsparser.execute(data)directly from the"data"event, so the exception escapes and crashes the process.Fix
Wrap
parser.execute(data)intry/catchand forward unexpected errors to the existingreturnFatalError -> recoverFromFatalErrorpath.This PR only handles the ioredis side. Adding a recursion limit belongs in
redis-parser.Test
test:js. One pre-existing flaky test (unrelated to this change) also fails on unmodifiedmain.Test plan
Fixes #2108
Note
Medium Risk
Touches the hot path for every inbound Redis byte and changes failure handling for parse errors; behavior is aligned with existing fatal recovery but affects connection lifecycle under attack or corruption.
Overview
Prevents process crashes when the Redis stream delivers a deeply nested RESP2 reply that makes
redis-parserthrow a synchronousRangeError(stack overflow) instead of using its normal fatal-parser callback.In
DataHandler, the stream"data"handler now wrapsparser.executeintry/catchand forwards caught errors through the existingreturnFatalError→recoverFromFatalErrorpath with{ offlineQueue: false }, matching behavior for other fatal protocol errors (flush in-flight commands, preserve offline queue, reconnect).A unit regression test feeds a 50k-level nested array buffer and asserts no throw, a single
recoverFromFatalErrorcall, and the expected error message/options.Reviewed by Cursor Bugbot for commit ac38e15. Bugbot is set up for automated code reviews on this repo. Configure here.