Skip to content

Avoid heap buffer overflow in valkeyAsyncFormattedCommand#245

Merged
bjosv merged 7 commits intovalkey-io:mainfrom
bjosv:fix-async-cmd-parsing-crash
Oct 16, 2025
Merged

Avoid heap buffer overflow in valkeyAsyncFormattedCommand#245
bjosv merged 7 commits intovalkey-io:mainfrom
bjosv:fix-async-cmd-parsing-crash

Conversation

@bjosv
Copy link
Copy Markdown
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 <bjorn.a.svensson@est.tech>
@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 <bjorn.a.svensson@est.tech>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
@michael-grunder
Copy link
Copy Markdown
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
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Co-authored-by: michael-grunder <michael.grunder@gmail.com>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
Signed-off-by: Björn Svensson <bjorn.a.svensson@est.tech>
@bjosv
Copy link
Copy Markdown
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.

@bjosv
Copy link
Copy Markdown
Collaborator Author

bjosv commented Oct 14, 2025

It seems that parseBulkLen was a nice improvement.
Just running nextArgument in a loop with a simple GET key command is faster now with this PR,
and just replacing strtol gave most of the improvement.

Before PR: 1000000x : 18.831ms
After PR:  1000000x : 14.889ms

@bjosv bjosv merged commit daa7f11 into valkey-io:main Oct 16, 2025
60 of 61 checks passed
@bjosv bjosv deleted the fix-async-cmd-parsing-crash branch October 16, 2025 08:04
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