wasphin commented on code in PR #3570:
URL: https://github.com/apache/brpc/pull/3570#discussion_r4178118323
##########
src/brpc/redis_command.cpp:
##########
@@ -257,10 +258,27 @@ RedisCommandFormatV(butil::IOBuf* outbuf, const char*
fmt, va_list ap) {
if (_l < sizeof(_format)-2) {
memcpy(_format, c, _l);
_format[_l] = '\0';
- int plen = vsnprintf(_printed, sizeof(_printed), _format,
_cpy);
- if (plen > 0) {
+ va_list _retry;
+ va_copy(_retry, _cpy);
+ const int plen = vsnprintf(_printed, sizeof(_printed),
_format, _cpy);
+ if (plen < 0) {
+ va_end(_retry);
+ va_end(_cpy);
+ return butil::Status(EINVAL, "Failed to format redis
command");
+ }
+ if (static_cast<size_t>(plen) >= sizeof(_printed)) {
+ std::vector<char> full(static_cast<size_t>(plen) + 1);
Review Comment:
The dynamic fallback fixes the OOB read, but it also allows the format
width/precision to directly control a potentially very large heap allocation.
Should we put an upper bound on plen (possibly consistent with
redis_max_allocation_size) before allocating?
This probably isn't blocking since fmt is already required to be trusted,
but bounding the allocation would make the failure mode safer.
--
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]
