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.

@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.

1 participant