Skip to content

fix redis: guard empty array reply in OnPsubscribeReply#1284

Open
netliomax25-code wants to merge 2 commits into
userver-framework:developfrom
netliomax25-code:redis-psubscribe-empty-array
Open

fix redis: guard empty array reply in OnPsubscribeReply#1284
netliomax25-code wants to merge 2 commits into
userver-framework:developfrom
netliomax25-code:redis-psubscribe-empty-array

Conversation

@netliomax25-code

Copy link
Copy Markdown
Contributor

Repro: psubscribe to a pattern and have the server answer the psubscribe with an empty array (RESP *0\r\n).
Cause: OnPsubscribeReply checks reply->data.IsArray() but then reads reply_array[0] before any size check, so an empty array is indexed out of bounds on the vector returned by GetArray(). The dispatch in subscription_storage.cpp only checks IsOk/non-nil/IsArray, so an empty array reaches this handler.
Fix: return early when the array is empty, matching the guard the sibling OnSubscribeImpl already applies before touching element 0. Added a regression test that feeds an empty-array PSUBSCRIBE reply and checks no callback fires and no out-of-bounds access happens under the addr;ub sanitizer build.

@hiSandog

hiSandog commented Jul 2, 2026

Copy link
Copy Markdown

The guard and regression test look focused. One adjacent case worth considering is whether the other pub/sub reply handlers can receive a non-empty but too-short array, for example an array with only the command string and no channel/count fields. If the parser accepts malformed RESP arrays from a server/proxy, a small table of malformed PSUBSCRIBE/PUNSUBSCRIBE/PMESSAGE replies would make this class of defensive parsing harder to regress.

@netliomax25-code

Copy link
Copy Markdown
Contributor Author

Good point. Two parts to it:

  1. The sibling paths were already safe against the too-short case. OnSubscribeImpl returns on reply_array.size() != 3 before touching element 0, and inside OnPsubscribeReply each branch only reads the channel/count fields when the size matches (3 for PSUBSCRIBE/PUNSUBSCRIBE, 4 for PMESSAGE). The only real gap was the unconditional reply_array[0] read at the top, which the empty-array guard now covers.

  2. Added the table anyway so this stays pinned. Sentinel.OnPsubscribeReplyTooShortArray feeds command-only and partial PSUBSCRIBE/PUNSUBSCRIBE/PMESSAGE replies and checks no callback fires and nothing indexes past the end. Both tests pass under the addr;ub sanitizer build.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants