fix(redis): harden delayed-member Lua decode against malformed members#143
Merged
Conversation
jotarios
added a commit
that referenced
this pull request
May 19, 2026
PROMOTE_SCRIPT and SCHEDULE_REPEATABLE_SCRIPT decoded the slice-3 length-prefixed delayed-ZSET member with string.byte(member, 1, 4) followed by name_len arithmetic and no bounds check. A member shorter than the 4-byte prefix made string.byte return nil; arithmetic on nil aborted the whole EVAL *before* the per-member ZREM, so the poison member was never removed and every promote tick re-read it and re-errored — delayed-job promotion for the queue was permanently wedged, also starving valid jobs scored at/after the poison. Mirror the existing Rust decoder (delayed_member::decode_delayed_member) in Lua: reject members shorter than the prefix and reject a declared name_len that overruns the buffer. In PROMOTE_SCRIPT a malformed member is ZREM'd (cleansed) and skipped; payloads uses its own dense counter so a skipped member can't punch a nil hole that truncates the reply for valid members after it. In SCHEDULE_REPEATABLE_SCRIPT a malformed member on the fire branch is dropped without counting toward fired/fired_now (it can't be dispatched, so it must not burn a fire slot against the repeatable limit); the loop still advances and the rest of the batch fires normally. Well-formed members hit the identical command sequence as before — no behavior change, no API/config change. Tests: new live-Redis regression poison_member_does_not_wedge_promotion (valid job still promoted, poison cleansed, ZSET drains, no wedge) plus in-crate structural guards so a future edit can't strip the check back out. Existing delayed/promoter/repeatable suites unchanged and green.
jotarios
force-pushed
the
fix/harden-delayed-member-lua-decode
branch
from
20d1734 to
28cfb29
Compare
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.
What
Hardens the two Lua scripts in
chasquimq/src/redis/commands.rsthat decode the slice-3 length-prefixed delayed-ZSET member (PROMOTE_SCRIPTandSCHEDULE_REPEATABLE_SCRIPT) so a malformed/short member is skipped and cleansed instead of permanently wedging promotion.Why
Both scripts decoded the member with
string.byte(member, 1, 4)followed byname_lenarithmetic and no bounds check. On a member shorter than the 4-byte length prefix,string.bytereturnsnilfor the missing positions; arithmetic onnilaborts the entire EVAL — and inPROMOTE_SCRIPTit aborts before the per-memberZREM. So the poison member is never removed, every subsequent promote tick re-reads it and re-errors, and delayed-job promotion for that queue is wedged forever (also starving valid jobs scored at/after the poison).The Rust decoder (
delayed_member::decode_delayed_member) already guards bothlen < 4andlen < 4 + name_len, returningNone. The Lua side now mirrors that exactly. This is internal robustness/correctness hardening — no public API change, no config change.Changes
PROMOTE_SCRIPT: bounds-check beforestring.byte. A member shorter than the prefix, or whose declaredname_lenoverruns the buffer, isZREM'd (cleansed) and skipped instead of aborting.payloadsnow uses its own dense counter (np) rather than the loop index, so a skipped member can't punch anilhole into the returned array (anilin a Lua array truncates the RESP reply, which would silently dropdidxcleanup for valid members after the poison).SCHEDULE_REPEATABLE_SCRIPT: same bounds guard on the fire branch. A poison member is dropped (it can't be dispatched) and explicitly does not count towardfired/fired_now— counting it would burn a fire slot against the repeatablelimitand could prematurely retire the spec. The loop still advances unconditionally; the rest of the batch fires normally. Well-formed members hit the identical command sequence as before.name_len == len - 4).Tests
delayed.rs::poison_member_does_not_wedge_promotion: ZADDs a 2-byte runt and an oversized-name_lenmember alongside a real delayed job, runs the promoter, asserts the valid job is promoted and consumed, both poison members are cleansed from the ZSET, and the ZSET drains to empty (proving repeated ticks keep working — no permanent wedge).cargo test --workspace -- --include-ignored→ 291 passed (34 suites) against live Redis 8.6.2.cargo fmt --all -- --checkandcargo clippy --all-targets --workspace -- -D warningsclean.Performance
worker-delayed-end-to-endA/B on the same host (scale=5, repeats=5, drop-slowest=1): main 17,249 jobs/s vs branch ~16.5k–17.4k jobs/s — within run-to-run stddev (300–790). The added per-member bounds checks are O(1) integer comparisons with zero extra Redis round trips or allocations. No measurable regression.