wwbmmm commented on code in PR #3566:
URL: https://github.com/apache/brpc/pull/3566#discussion_r4104698902
##########
src/brpc/redis_command.cpp:
##########
@@ -147,24 +153,40 @@ 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*);
+ // Most callers pass const char* (c_str()/data()); reading as
+ // const char* matches them exactly. char* is ABI-identical to
+ // const char*, so mutable callers stay correct in practice; a
+ // single va_arg cannot serve both types exactly.
+ 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");
Review Comment:
[replied by brpc-oncall robot] These new EINVAL returns are correct at the
`RedisCommandFormat` level, but when the entry point is
`RedisRequest::AddCommand*` they hit the pre-existing `CHECK(st.ok()) << st;`
in the failure branch of
`AddCommand`/`AddCommandV`/`AddCommandWithArgs`/`AddCommandByComponents`
(src/brpc/redis.cpp). That CHECK sits in the `else` branch where the status is
by definition not OK, so it always fires and aborts the process before
`_has_error` is set. So `request.AddCommand("set key %s",
ptr_that_may_be_null)` still terminates the program (with a CHECK log instead
of a strlen(NULL) crash), which is why the new test only covers the direct
`RedisCommandFormat` path. Please confirm the intent: either these errors are
fatal-by-design on the AddCommand path (then please don't claim a crash is
avoided), or that stray `CHECK` should be dropped so callers get
`false`/`has_error()` as the header documents. Same applies to the `%b` NULL
error below.
--
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]
