two-headBoy commented on code in PR #3566:
URL: https://github.com/apache/brpc/pull/3566#discussion_r4102295380
##########
src/brpc/redis_command.cpp:
##########
@@ -147,24 +153,38 @@ RedisCommandFormatV(butil::IOBuf* outbuf, const char*
fmt, va_list ap) {
compbuf.push_back(*c);
}
} else {
- char *arg;
+ const char* arg;
size_t size;
switch(c[1]) {
case 's':
- arg = va_arg(ap, char*);
+ // Callers pass const char* (c_str()/data()); read it back with
+ // the exact type to avoid UB at the variadic boundary.
+ arg = va_arg(ap, const char*);
+ // strlen(NULL) is UB; forbid it explicitly instead of
crashing.
+ if (arg == nullptr) {
+ return butil::Status(EINVAL, "%%s argument is NULL");
+ }
size = strlen(arg);
if (size > 0) {
compbuf.append(arg, size);
}
+ format_arg_in_comp = true;
++nargs;
break;
case 'b':
- arg = va_arg(ap, char*);
+ arg = va_arg(ap, const char*);
Review Comment:
A single`va_arg` cannot be strictly correct for both`char*` and`const char*`
at the same time. To eliminate this entirely, the pointer arguments would have
to be taken out of the variadic list — i.e., via typed overloads
(changing`AddCommand` into typed overloads that no longer go through`...` ).
That is a fairly large API refactor, far beyond the scope of #2275 (the
empty-argument bug).
As for the "mutable-buffer test" it requested — adding a case that passes
a`char*` just exercises the same code path once more, and it cannot detect this
formal UB (because the two are ABI-identical), so it provides no real help
toward the fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
