From f913e9b997e2cc4b847923e1b3b0649d77c85923 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Thu, 15 Oct 2020 17:46:15 -0400 Subject: read: Fix double validation and infinity parsing The ',' protocol byte gets removed in processItem(), so it should not be compared against in processLineItem(). strtod() allows multiple representations of infinity and NaN that are not RESP3 compliant. Since we explicitly check for the two compliant infinity cases, strtod() should only return finite values. --- read.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'read.c') diff --git a/read.c b/read.c index 0952469..34fce2e 100644 --- a/read.c +++ b/read.c @@ -299,13 +299,17 @@ static int processLineItem(redisReader *r) { memcpy(buf,p,len); buf[len] = '\0'; - if (strcasecmp(buf,",inf") == 0) { + if (strcasecmp(buf,"inf") == 0) { d = INFINITY; /* Positive infinite. */ - } else if (strcasecmp(buf,",-inf") == 0) { + } else if (strcasecmp(buf,"-inf") == 0) { d = -INFINITY; /* Negative infinite. */ } else { d = strtod((char*)buf,&eptr); - if (buf[0] == '\0' || eptr[0] != '\0' || isnan(d)) { + /* RESP3 only allows "inf", "-inf", and finite values, while + * strtod() allows other variations on infinity, NaN, + * etc. We explicity handle our two allowed infinite cases + * above, so strtod() should only result in finite values. */ + if (buf[0] == '\0' || eptr[0] != '\0' || !isfinite(d)) { __redisReaderSetError(r,REDIS_ERR_PROTOCOL, "Bad double value"); return REDIS_ERR; -- cgit v1.2.3 From d8899fbc190d49bb09c2576035d4db4ead185846 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Thu, 15 Oct 2020 17:54:58 -0400 Subject: read: Add additional RESP3 nil validation RESP3 nil should consist of "_\r\n" and nothing else. --- read.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'read.c') diff --git a/read.c b/read.c index 34fce2e..57da853 100644 --- a/read.c +++ b/read.c @@ -320,6 +320,12 @@ static int processLineItem(redisReader *r) { obj = (void*)REDIS_REPLY_DOUBLE; } } else if (cur->type == REDIS_REPLY_NIL) { + if (len != 0) { + __redisReaderSetError(r,REDIS_ERR_PROTOCOL, + "Bad nil value"); + return REDIS_ERR; + } + if (r->fn && r->fn->createNil) obj = r->fn->createNil(cur); else -- cgit v1.2.3 From 51e693f4fd54d6a9dfbdafa1b361f6618e4f7ba3 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Thu, 15 Oct 2020 17:55:30 -0400 Subject: read: Add additional RESP3 bool validation RESP3 bools should be only one of "#t\r\n" or "#f\r\n". We also allow capital 'T' and 'F' to be lenient. --- read.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) (limited to 'read.c') diff --git a/read.c b/read.c index 57da853..1378671 100644 --- a/read.c +++ b/read.c @@ -331,7 +331,15 @@ static int processLineItem(redisReader *r) { else obj = (void*)REDIS_REPLY_NIL; } else if (cur->type == REDIS_REPLY_BOOL) { - int bval = p[0] == 't' || p[0] == 'T'; + int bval; + + if (len != 1 || !strchr("tTfF", p[0])) { + __redisReaderSetError(r,REDIS_ERR_PROTOCOL, + "Bad bool value"); + return REDIS_ERR; + } + + bval = p[0] == 't' || p[0] == 'T'; if (r->fn && r->fn->createBool) obj = r->fn->createBool(cur,bval); else -- cgit v1.2.3 From 397fe2630170e33eee06533749b1802f9371cb3e Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Thu, 15 Oct 2020 19:17:58 -0400 Subject: read: Use memchr() in seekNewline() instead of looping over entire string --- read.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) (limited to 'read.c') diff --git a/read.c b/read.c index 1378671..22d6e47 100644 --- a/read.c +++ b/read.c @@ -123,29 +123,22 @@ static char *readBytes(redisReader *r, unsigned int bytes) { /* Find pointer to \r\n. */ static char *seekNewline(char *s, size_t len) { - int pos = 0; + char *_s = s, *ret; int _len = len-1; - /* Position should be < len-1 because the character at "pos" should be - * followed by a \n. Note that strchr cannot be used because it doesn't - * allow to search a limited length and the buffer that is being searched - * might not have a trailing NULL character. */ - while (pos < _len) { - while(pos < _len && s[pos] != '\r') pos++; - if (pos==_len) { - /* Not found. */ - return NULL; - } else { - if (s[pos+1] == '\n') { - /* Found. */ - return s+pos; - } else { - /* Continue searching. */ - pos++; - } + /* Exclude the last character from the searched length because the found + * '\r' should be followed by a '\n' */ + while ((ret = memchr(_s, '\r', _len)) != NULL) { + if (ret[1] == '\n') { + /* Found. */ + break; } + /* Continue searching. */ + ret++; + _len -= ret - _s; + _s = ret; } - return NULL; + return ret; } /* Convert a string into a long long. Returns REDIS_OK if the string could be -- cgit v1.2.3 From e43061156cffea910ab46dab1135e16e70774dce Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 16 Oct 2020 17:32:38 -0400 Subject: read: Additional validation and test case for RESP3 double This ensures that malformed RESP3 double messages that include an invalid null byte are not parsed as valid. --- read.c | 6 +++--- test.c | 9 +++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) (limited to 'read.c') diff --git a/read.c b/read.c index 22d6e47..01fa9dc 100644 --- a/read.c +++ b/read.c @@ -292,9 +292,9 @@ static int processLineItem(redisReader *r) { memcpy(buf,p,len); buf[len] = '\0'; - if (strcasecmp(buf,"inf") == 0) { + if (len == 3 && strcasecmp(buf,"inf") == 0) { d = INFINITY; /* Positive infinite. */ - } else if (strcasecmp(buf,"-inf") == 0) { + } else if (len == 4 && strcasecmp(buf,"-inf") == 0) { d = -INFINITY; /* Negative infinite. */ } else { d = strtod((char*)buf,&eptr); @@ -302,7 +302,7 @@ static int processLineItem(redisReader *r) { * strtod() allows other variations on infinity, NaN, * etc. We explicity handle our two allowed infinite cases * above, so strtod() should only result in finite values. */ - if (buf[0] == '\0' || eptr[0] != '\0' || !isfinite(d)) { + if (buf[0] == '\0' || eptr != &buf[len] || !isfinite(d)) { __redisReaderSetError(r,REDIS_ERR_PROTOCOL, "Bad double value"); return REDIS_ERR; diff --git a/test.c b/test.c index baa5a87..fa861b3 100644 --- a/test.c +++ b/test.c @@ -597,6 +597,15 @@ static void test_reply_reader(void) { freeReplyObject(reply); redisReaderFree(reader); + test("Set error on invalid RESP3 double: "); + reader = redisReaderCreate(); + redisReaderFeed(reader, ",3.14159\000265358979323846\r\n",26); + ret = redisReaderGetReply(reader,&reply); + test_cond(ret == REDIS_ERR && + strcasecmp(reader->errstr,"Bad double value") == 0); + freeReplyObject(reply); + redisReaderFree(reader); + test("Correctly parses RESP3 double INFINITY: "); reader = redisReaderCreate(); redisReaderFeed(reader, ",inf\r\n",6); -- cgit v1.2.3 From c6646cb19242b0e0966760b38e7df74742b3a7b2 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 16 Oct 2020 17:38:38 -0400 Subject: read: Ensure no invalid '\r' or '\n' in simple status/error strings --- read.c | 7 +++++++ 1 file changed, 7 insertions(+) (limited to 'read.c') diff --git a/read.c b/read.c index 01fa9dc..89b5b5a 100644 --- a/read.c +++ b/read.c @@ -339,6 +339,13 @@ static int processLineItem(redisReader *r) { obj = (void*)REDIS_REPLY_BOOL; } else { /* Type will be error or status. */ + for (int i = 0; i < len; i++) { + if (p[i] == '\r' || p[i] == '\n') { + __redisReaderSetError(r,REDIS_ERR_PROTOCOL, + "Bad simple string value"); + return REDIS_ERR; + } + } if (r->fn && r->fn->createString) obj = r->fn->createString(cur,p,len); else -- cgit v1.2.3 From 83c14504257de168c45ae7730a00c930ab17cfa3 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 16 Oct 2020 18:35:58 -0400 Subject: read: Add support for the RESP3 bignum type --- hiredis.c | 4 +++- hiredis.h | 3 ++- read.c | 20 ++++++++++++++++++++ test.c | 11 +++++++++++ 4 files changed, 36 insertions(+), 2 deletions(-) (limited to 'read.c') diff --git a/hiredis.c b/hiredis.c index 5591cb9..2c1fb82 100644 --- a/hiredis.c +++ b/hiredis.c @@ -114,6 +114,7 @@ void freeReplyObject(void *reply) { case REDIS_REPLY_STRING: case REDIS_REPLY_DOUBLE: case REDIS_REPLY_VERB: + case REDIS_REPLY_BIGNUM: hi_free(r->str); break; } @@ -131,7 +132,8 @@ static void *createStringObject(const redisReadTask *task, char *str, size_t len assert(task->type == REDIS_REPLY_ERROR || task->type == REDIS_REPLY_STATUS || task->type == REDIS_REPLY_STRING || - task->type == REDIS_REPLY_VERB); + task->type == REDIS_REPLY_VERB || + task->type == REDIS_REPLY_BIGNUM); /* Copy string value */ if (task->type == REDIS_REPLY_VERB) { diff --git a/hiredis.h b/hiredis.h index a629930..e77a88a 100644 --- a/hiredis.h +++ b/hiredis.h @@ -112,7 +112,8 @@ typedef struct redisReply { double dval; /* The double when type is REDIS_REPLY_DOUBLE */ size_t len; /* Length of string */ char *str; /* Used for REDIS_REPLY_ERROR, REDIS_REPLY_STRING - REDIS_REPLY_VERB, and REDIS_REPLY_DOUBLE (in additional to dval). */ + REDIS_REPLY_VERB, REDIS_REPLY_DOUBLE (in additional to dval), + and REDIS_REPLY_BIGNUM. */ char vtype[4]; /* Used for REDIS_REPLY_VERB, contains the null terminated 3 character content type, such as "txt". */ size_t elements; /* number of elements, for REDIS_REPLY_ARRAY */ diff --git a/read.c b/read.c index 89b5b5a..5e0e0b4 100644 --- a/read.c +++ b/read.c @@ -337,6 +337,22 @@ static int processLineItem(redisReader *r) { obj = r->fn->createBool(cur,bval); else obj = (void*)REDIS_REPLY_BOOL; + } else if (cur->type == REDIS_REPLY_BIGNUM) { + /* Ensure all characters are decimal digits (with possible leading + * minus sign). */ + for (int i = 0; i < len; i++) { + /* XXX Consider: Allow leading '+'? Error on leading '0's? */ + if (i == 0 && p[0] == '-') continue; + if (p[i] < '0' || p[i] > '9') { + __redisReaderSetError(r,REDIS_ERR_PROTOCOL, + "Bad bignum value"); + return REDIS_ERR; + } + } + if (r->fn && r->fn->createString) + obj = r->fn->createString(cur,p,len); + else + obj = (void*)REDIS_REPLY_BIGNUM; } else { /* Type will be error or status. */ for (int i = 0; i < len; i++) { @@ -587,6 +603,9 @@ static int processItem(redisReader *r) { case '>': cur->type = REDIS_REPLY_PUSH; break; + case '(': + cur->type = REDIS_REPLY_BIGNUM; + break; default: __redisReaderSetErrorProtocolByte(r,*p); return REDIS_ERR; @@ -605,6 +624,7 @@ static int processItem(redisReader *r) { case REDIS_REPLY_DOUBLE: case REDIS_REPLY_NIL: case REDIS_REPLY_BOOL: + case REDIS_REPLY_BIGNUM: return processLineItem(r); case REDIS_REPLY_STRING: case REDIS_REPLY_VERB: diff --git a/test.c b/test.c index fa861b3..f830695 100644 --- a/test.c +++ b/test.c @@ -714,6 +714,17 @@ static void test_reply_reader(void) { ((redisReply*)reply)->element[4]->integer == 999); freeReplyObject(reply); redisReaderFree(reader); + + test("Can parse RESP3 bignum: "); + reader = redisReaderCreate(); + redisReaderFeed(reader,"(3492890328409238509324850943850943825024385\r\n",46); + ret = redisReaderGetReply(reader,&reply); + test_cond(ret == REDIS_OK && + ((redisReply*)reply)->type == REDIS_REPLY_BIGNUM && + ((redisReply*)reply)->len == 43 && + !strcmp(((redisReply*)reply)->str,"3492890328409238509324850943850943825024385")); + freeReplyObject(reply); + redisReaderFree(reader); } static void test_free_null(void) { -- cgit v1.2.3 From 5f9242a1f8d99bf9b7864e8ecdce443fe821ab40 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 16 Oct 2020 18:36:52 -0400 Subject: read: Remove obsolete comment on nested multi bulk depth limitation --- read.c | 1 - 1 file changed, 1 deletion(-) (limited to 'read.c') diff --git a/read.c b/read.c index 5e0e0b4..9759dbc 100644 --- a/read.c +++ b/read.c @@ -487,7 +487,6 @@ static int processAggregateItem(redisReader *r) { long long elements; int root = 0, len; - /* Set error for nested multi bulks with depth > 7 */ if (r->ridx == r->tasks - 1) { if (redisReaderGrow(r) == REDIS_ERR) return REDIS_ERR; -- cgit v1.2.3 From bd7488d27d235a35a16519446173bef99fd4f200 Mon Sep 17 00:00:00 2001 From: Alex Smith Date: Fri, 16 Oct 2020 18:57:56 -0400 Subject: read: Validate line items prior to checking for object creation callbacks --- read.c | 63 +++++++++++++++++++++++++++++++++------------------------------ 1 file changed, 33 insertions(+), 30 deletions(-) (limited to 'read.c') diff --git a/read.c b/read.c index 9759dbc..24c2b58 100644 --- a/read.c +++ b/read.c @@ -267,47 +267,50 @@ static int processLineItem(redisReader *r) { if ((p = readLine(r,&len)) != NULL) { if (cur->type == REDIS_REPLY_INTEGER) { + long long v; + + if (string2ll(p, len, &v) == REDIS_ERR) { + __redisReaderSetError(r,REDIS_ERR_PROTOCOL, + "Bad integer value"); + return REDIS_ERR; + } + if (r->fn && r->fn->createInteger) { - long long v; - if (string2ll(p, len, &v) == REDIS_ERR) { - __redisReaderSetError(r,REDIS_ERR_PROTOCOL, - "Bad integer value"); - return REDIS_ERR; - } obj = r->fn->createInteger(cur,v); } else { obj = (void*)REDIS_REPLY_INTEGER; } } else if (cur->type == REDIS_REPLY_DOUBLE) { - if (r->fn && r->fn->createDouble) { - char buf[326], *eptr; - double d; + char buf[326], *eptr; + double d; - if ((size_t)len >= sizeof(buf)) { + if ((size_t)len >= sizeof(buf)) { + __redisReaderSetError(r,REDIS_ERR_PROTOCOL, + "Double value is too large"); + return REDIS_ERR; + } + + memcpy(buf,p,len); + buf[len] = '\0'; + + if (len == 3 && strcasecmp(buf,"inf") == 0) { + d = INFINITY; /* Positive infinite. */ + } else if (len == 4 && strcasecmp(buf,"-inf") == 0) { + d = -INFINITY; /* Negative infinite. */ + } else { + d = strtod((char*)buf,&eptr); + /* RESP3 only allows "inf", "-inf", and finite values, while + * strtod() allows other variations on infinity, NaN, + * etc. We explicity handle our two allowed infinite cases + * above, so strtod() should only result in finite values. */ + if (buf[0] == '\0' || eptr != &buf[len] || !isfinite(d)) { __redisReaderSetError(r,REDIS_ERR_PROTOCOL, - "Double value is too large"); + "Bad double value"); return REDIS_ERR; } + } - memcpy(buf,p,len); - buf[len] = '\0'; - - if (len == 3 && strcasecmp(buf,"inf") == 0) { - d = INFINITY; /* Positive infinite. */ - } else if (len == 4 && strcasecmp(buf,"-inf") == 0) { - d = -INFINITY; /* Negative infinite. */ - } else { - d = strtod((char*)buf,&eptr); - /* RESP3 only allows "inf", "-inf", and finite values, while - * strtod() allows other variations on infinity, NaN, - * etc. We explicity handle our two allowed infinite cases - * above, so strtod() should only result in finite values. */ - if (buf[0] == '\0' || eptr != &buf[len] || !isfinite(d)) { - __redisReaderSetError(r,REDIS_ERR_PROTOCOL, - "Bad double value"); - return REDIS_ERR; - } - } + if (r->fn && r->fn->createDouble) { obj = r->fn->createDouble(cur,d,buf,len); } else { obj = (void*)REDIS_REPLY_DOUBLE; -- cgit v1.2.3 From 410c24d2a977288ec97ffac23a303597b9f98d67 Mon Sep 17 00:00:00 2001 From: michael-grunder Date: Mon, 19 Oct 2020 10:11:51 -0700 Subject: Fix off-by-one error in seekNewline --- read.c | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) (limited to 'read.c') diff --git a/read.c b/read.c index 24c2b58..de62b9a 100644 --- a/read.c +++ b/read.c @@ -123,21 +123,27 @@ static char *readBytes(redisReader *r, unsigned int bytes) { /* Find pointer to \r\n. */ static char *seekNewline(char *s, size_t len) { - char *_s = s, *ret; - int _len = len-1; + char *ret; - /* Exclude the last character from the searched length because the found - * '\r' should be followed by a '\n' */ - while ((ret = memchr(_s, '\r', _len)) != NULL) { + /* We cannot match with fewer than 2 bytes */ + if (len < 2) + return NULL; + + /* Search up to len - 1 characters */ + len--; + + /* Look for the \r */ + while ((ret = memchr(s, '\r', len)) != NULL) { if (ret[1] == '\n') { /* Found. */ break; } /* Continue searching. */ ret++; - _len -= ret - _s; - _s = ret; + len -= ret - s; + s = ret; } + return ret; } -- cgit v1.2.3