cachedb_redis: fix safety issues in cluster redirect parsing#3854
Open
NormB wants to merge 1 commit intoOpenSIPS:masterfrom
Open
cachedb_redis: fix safety issues in cluster redirect parsing#3854NormB wants to merge 1 commit intoOpenSIPS:masterfrom
NormB wants to merge 1 commit intoOpenSIPS:masterfrom
Conversation
Fix several correctness and safety issues in parse_moved_reply() and the MOVED redirect handler: - Add slot value overflow protection: return ERR_INVALID_SLOT when parsed slot exceeds 16383 during digit accumulation, preventing signed integer overflow on malformed MOVED replies. - Add port value overflow protection: return ERR_INVALID_PORT when parsed port exceeds 65535 during digit accumulation, complementing the existing post-loop range check and preventing signed integer overflow on malformed input. - Fix undefined behavior in the no-colon endpoint fallback path: replace comparison of potentially-NULL out->endpoint.s against end pointer with (p < end), which achieves the same logic using the scan position variable that is always valid. - Replace pkg_malloc heap allocation of redis_moved struct with stack allocation in the MOVED handler. The struct is small (~24 bytes) and never outlives the enclosing scope, making heap allocation unnecessary. This eliminates the OOM error path and two pkg_free() calls.
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
1 participant
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
Fix several correctness and safety issues in
parse_moved_reply()and the MOVED redirect handler.Details
Integer overflow in slot parsing — the digit accumulation loop
slot = slot * 10 + (*p - '0')has no upper bound check. A malformed MOVED reply with a long digit string causes signed integer overflow (undefined behavior in C). Fixed by returningERR_INVALID_SLOTwhenslot > 16383during parsing.Integer overflow in port parsing — same issue in the port digit loop. The existing post-loop check
port > 65535is insufficient because signed overflow is UB before the check executes. Fixed by returningERR_INVALID_PORTwhenport > 65535during digit accumulation.Undefined behavior in no-colon fallback — when the endpoint has no colon (no explicit port), the code compares
out->endpoint.s < endwhereout->endpoint.swas set toNULLtwo lines above. Comparing a null pointer to a non-null pointer is undefined behavior per the C standard. Fixed by usingp < endinstead, which holds the correct scan position.Unnecessary heap allocation — the
redis_movedstruct in the MOVED handler is heap-allocated viapkg_mallocand freed within the same scope. Since the struct is ~24 bytes and never outlives the enclosing block, this is replaced with stack allocation, eliminating thepkg_malloc/pkg_freecalls and the OOM error path.Compatibility
No behavioral change for correctly-formed Redis replies. The overflow checks add early rejection of malformed input that would previously have caused undefined behavior.
Closing issues
Partially addresses #2811