Skip to content

Conversation

bjosv
Copy link
Collaborator

@bjosv bjosv commented Oct 2, 2025

valkeyAsyncFormattedCommand now returns VALKEY_ERR instead of crashing when the command length, or content, is faulty.

Adds validation of the parsed length to make sure we don't read past the buffer end.
The internal nextArgument function now takes a new function argument, the buffer length, to be able to do the validation.

Fixes #242

valkeyAsyncFormattedCommand returns VALKEY_ERR instead of
asserting when the command length, or content, is faulty.

Validate parsed length and make sure we don't read past
the buffer end.

Signed-off-by: Björn Svensson <[email protected]>
@bjosv bjosv requested a review from michael-grunder October 2, 2025 12:01
bjosv added 2 commits October 2, 2025 14:03
Signed-off-by: Björn Svensson <[email protected]>
Signed-off-by: Björn Svensson <[email protected]>
@michael-grunder
Copy link
Collaborator

michael-grunder commented Oct 3, 2025

This is great.

The only big questions I have are around bulk length (e.g. $0\r\n\r\n or $-1\r\n). I don't think clients are ever supposed to send null bulk arguments so that's probably fine to ignore.

If we are only ever going to accept len >= 0 we could be slightly more paranoid and use a bespoke parser that does it in place while taking an end pointer.

I played around with something like that here:
https://github.com/michael-grunder/libvalkey/blob/2e13a4ef0d64ab32de0aee4522b8f7fed5c68733/src/async.c#L866

That might be overkill though 😄

bjosv and others added 4 commits October 3, 2025 11:59
@bjosv
Copy link
Collaborator Author

bjosv commented Oct 3, 2025

I incorporated your length parser now so the PR got a bit bigger, but I think its more straight on.
I'll see if I can run some benchmark comparisons.

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.

[BUG] Heap OOB read in valkeyAsyncAppendCmdLen when processing non-NUL-terminated formatted command buffer
2 participants