-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix potential fault at createDoubleObject #964
Conversation
hiredis.c
Outdated
@@ -231,6 +231,11 @@ static void *createDoubleObject(const redisReadTask *task, double value, char *s | |||
freeReplyObject(r); | |||
return NULL; | |||
} | |||
else if (len+1 == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case would len be -1 here as described in #963 ?
Ien is a size_t, i.e. an unsigned integer, and when the function is called using a signed integer the conversion rules
in 6.3.1.3 (bullet 2) of the c99 standard would apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to protect against hi_malloc(len + 1)
when len == SIZE_MAX
but I wonder if that's the more straightforward way to do it.
Just test that at the top of the routine and return NULL
if it is true.
If we're feeling ambitious I suppose we could limit the maximum value here to a much more sane length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, yes, that's a potential issue. Testing in the top of the routine would be simple and pragmatic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about confusing with -1
.
This function is currently not able to be called if parameter len
is larger than 326.
Lines 290 to 297 in dfa33e6
char buf[326], *eptr; | |
double d; | |
if ((size_t)len >= sizeof(buf)) { | |
__redisReaderSetError(r,REDIS_ERR_PROTOCOL, | |
"Double value is too large"); | |
return REDIS_ERR; | |
} |
However, this check depends on the buffer size, which seems not so stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If len < 326 at this point, it means the added return NULL;
is dead code. Better add an assert(len < SIZE_MAX)
instead then.
Resolves redis#963. Add additional check to `hi_malloc` for `r->str` when len equals to SIZE_MAX.
5413631
to
50cdcab
Compare
Resolves #963.
Add additional check to
hi_malloc
forr->str
when len+1 equals to 0.