Skip to content
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

Potential fault at createDoubleObject #963

Closed
afcidk opened this issue Jun 14, 2021 · 1 comment · Fixed by #964
Closed

Potential fault at createDoubleObject #963

afcidk opened this issue Jun 14, 2021 · 1 comment · Fixed by #964

Comments

@afcidk
Copy link
Contributor

afcidk commented Jun 14, 2021

The potential fault occurs at the parameter len of createDoubleObject.

From the function prototype, we can see that len is of size_t type, which is known as an unsigned integer.

static void *createDoubleObject(const redisReadTask *task, double value, char *str, size_t len) {

This function attempts to allocate some space for string-typed double value dval, and allocates one more byte for the null-terminating character for the string.

hiredis/hiredis.c

Lines 228 to 233 in b6f86f3

r->dval = value;
r->str = hi_malloc(len+1);
if (r->str == NULL) {
freeReplyObject(r);
return NULL;
}

When len is -1, the program allocates a space with size 0 for r->str.
According to man 3 malloc,

If size is 0, then malloc() returns either NULL, or a unique pointer value that can later be successfully passed to free().

If hi_malloc does not return NULL, the program will crash in the following memcpy, which passes maximum value of unsigned integer to the size of memcpy.

memcpy(r->str, str, len);

Additional check to len parameter should be included in createDoubleObject function.

afcidk added a commit to afcidk/hiredis that referenced this issue Jun 14, 2021
Resolves redis#963.

Add additional check after `hi_malloc` for r->str when len+1 equals to
0.
afcidk added a commit to afcidk/hiredis that referenced this issue Jun 14, 2021
Resolves redis#963.

Add additional check to `hi_malloc` for `r->str` when len+1 equals to 0.
afcidk added a commit to afcidk/hiredis that referenced this issue Jun 14, 2021
Resolves redis#963.

Add additional check to `hi_malloc` for `r->str` when len+1 equals to 0.
@kristjanvalur
Copy link
Contributor

Why would you want to specify len as -1 (it is unsigned). Or do you mean to say that the largest possible len value causes a crash? Wouldn't it simply be better to specify the behaviour as undefined if len + 1 does not fit within a size_t ?

afcidk added a commit to afcidk/hiredis that referenced this issue Nov 1, 2021
Resolves redis#963.

Add additional check to `hi_malloc` for `r->str` when len equals to
SIZE_MAX.
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 a pull request may close this issue.

2 participants