Bug: missing return value check may cause NULL Pointer Dereference.

[MicroMilo]

hiredis/sds.c

Lines 376 to 390 in e834b57

	/* Append the specified binary-safe string pointed by 't' of 'len' bytes to the
	* end of the specified sds string 's'.
	*
	* After the call, the passed sds string is no longer valid and all the
	* references must be substituted with the new pointer returned by the call. */
	sds sdscatlen(sds s, const void *t, size_t len) {
	size_t curlen = sdslen(s);
	s = sdsMakeRoomFor(s,len);
	if (s == NULL) return NULL;
	memcpy(s+curlen, t, len);
	sdssetlen(s, curlen+len);
	s[curlen+len] = '\0';
	return s;
	}

Function sdscatlen calls sdsMakeRoomFor, which may fail due to memory management.
So there exists a potential NULL Pointer Dereference if we do check the return value of it.

We should check if s is NULL and then apply the follow-up operations, because the sdslen will dereference s directly.

hiredis/sds.c

Lines 875 to 898 in e834b57

	sds sdscatrepr(sds s, const char *p, size_t len) {
	s = sdscatlen(s,"\"",1);
	while(len--) {
	switch(*p) {
	case '\\':
	case '"':
	s = sdscatprintf(s,"\\%c",*p);
	break;
	case '\n': s = sdscatlen(s,"\\n",2); break;
	case '\r': s = sdscatlen(s,"\\r",2); break;
	case '\t': s = sdscatlen(s,"\\t",2); break;
	case '\a': s = sdscatlen(s,"\\a",2); break;
	case '\b': s = sdscatlen(s,"\\b",2); break;
	default:
	if (isprint((int) *p))
	s = sdscatprintf(s,"%c",*p);
	else
	s = sdscatprintf(s,"\\x%02x",(unsigned char)*p);
	break;
	}
	p++;
	}
	return sdscatlen(s,"\"",1);
	}

[MicroMilo]

sdsempty() suffers the same problem.

hiredis/sds.c

Lines 958 to 969 in e834b57

	if (current == NULL) current = sdsempty();
	while(!done) {
	if (inq) {
	if (*p == '\\' && *(p+1) == 'x' &&
	isxdigit((int) *(p+2)) &&
	isxdigit((int) *(p+3)))
	{
	unsigned char byte;
	byte = (hex_digit_to_int(*(p+2))*16)+
	hex_digit_to_int(*(p+3));
	current = sdscatlen(current,(char*)&byte,1);

hiredis/sds.c

Lines 1086 to 1091 in e834b57

	sds sdsjoin(char **argv, int argc, char *sep) {
	sds join = sdsempty();
	int j;
	for (j = 0; j < argc; j++) {
	join = sdscat(join, argv[j]);

hiredis/sds.c

Lines 1098 to 1103 in e834b57

	sds sdsjoinsds(sds *argv, int argc, const char *sep, size_t seplen) {
	sds join = sdsempty();
	int j;
	for (j = 0; j < argc; j++) {
	join = sdscatsds(join, argv[j]);

[michael-grunder]

One issue here is that historically hiredis shares sds with Redis itself.

I have to take a closer look, but I don't believe we call sdscatrepr, sdssplitlen, sdsjoin, or sdsjoinsds in hiredis library code.