Skip to content

Fix empty %b/%s argument dropped in redis command formatting (#2275) - #3566

Open
two-headBoy wants to merge 5 commits into
apache:masterfrom
two-headBoy:master
Open

two-headBoy wants to merge 5 commits into
apache:masterfrom
two-headBoy:master

Conversation

@two-headBoy

Copy link
Copy Markdown

What problem does this PR solve?

Issue Number: resolve #2275

Problem Summary:
An empty value passed as a %b/%s argument was silently dropped, producing a Redis command with the wrong number of arguments and causing "wrong number of arguments" errors.

What is changed and the side effects?

Changed:

  • Empty %b/%s arguments now emit an empty bulk string instead of being dropped.
  • %s with NULL, and %b with NULL and non-zero size, return an error to avoid undefined behavior.
  • Added unittests.

Side effects:

  • Performance effects: none.

  • Breaking backward compatibility: none.


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

A critical variadic pointer-type issue and a moderate binary-data test gap remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 1 Medium severity

Open (2)
What changed in this PR

Fixes Redis %s/%b formatting so empty arguments produce empty bulk strings and invalid null arguments return errors.

Changes:

  • Preserves empty format arguments.
  • Adds null-pointer validation.
  • Adds regression tests for empty and binary arguments.
File Summary
test/​brpc_redis_unittest.cpp Adds regression coverage; binary payload assertion should compare serialized bytes. Moderate issue (4 votes).
src/​brpc/​redis_command.cpp Updates argument handling and validation; variadic pointer typing causes undefined behavior. Critical issue (1 vote).

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/brpc/redis_command.cpp Outdated
Comment thread test/brpc_redis_unittest.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Address the variadic pointer compatibility issue and strengthen binary payload test coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread src/brpc/redis_command.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Resolve the variadic pointer type mismatches for mutable %s and %b buffers.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 High severity

Open (2)

Comment thread src/brpc/redis_command.cpp

@wwbmmm wwbmmm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[replied by brpc-oncall robot] The empty %s/%b fix is correct and the binary test now compares full bytes; the variadic char*/const char* concern is now documented as a pre-existing, unfixable-without-refactor limitation of this API, which I agree is out of scope here, and the only remaining observation is that the new EINVAL paths still abort when reached through RedisRequest::AddCommand.

Comment thread src/brpc/redis_command.cpp
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.

brpc redis客户端不支持空字符串

3 participants