From 60a4f12f8b998c44dfff0e88202b01598287390d Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Thu, 21 Dec 2017 11:10:48 +0200 Subject: [PATCH] fix processing of large bulks (above 2GB) - protocol parsing (processMultibulkBuffer) was limitted to 32big positions in the buffer readQueryFromClient potential overflow - rioWriteBulkCount used int, although rioWriteBulkString gave it size_t - several places in sds.c that used int for string length or index. - bugfix in RM_SaveAuxField (return was 1 or -1 and not length) - RM_SaveStringBuffer was limitted to 32bit length --- src/module.c | 4 ++-- src/networking.c | 13 +++++++------ src/rdb.c | 20 ++++++++++++-------- src/rdb.h | 2 +- src/rio.c | 2 +- src/rio.h | 2 +- src/sds.c | 23 ++++++++++++----------- src/sds.h | 6 +++--- 8 files changed, 39 insertions(+), 33 deletions(-) diff --git a/src/module.c b/src/module.c index 8a4c40f1..97818802 100644 --- a/src/module.c +++ b/src/module.c @@ -3024,7 +3024,7 @@ int64_t RM_LoadSigned(RedisModuleIO *io) { void RM_SaveString(RedisModuleIO *io, RedisModuleString *s) { if (io->error) return; /* Save opcode. */ - int retval = rdbSaveLen(io->rio, RDB_MODULE_OPCODE_STRING); + ssize_t retval = rdbSaveLen(io->rio, RDB_MODULE_OPCODE_STRING); if (retval == -1) goto saveerr; io->bytes += retval; /* Save value. */ @@ -3042,7 +3042,7 @@ saveerr: void RM_SaveStringBuffer(RedisModuleIO *io, const char *str, size_t len) { if (io->error) return; /* Save opcode. */ - int retval = rdbSaveLen(io->rio, RDB_MODULE_OPCODE_STRING); + ssize_t retval = rdbSaveLen(io->rio, RDB_MODULE_OPCODE_STRING); if (retval == -1) goto saveerr; io->bytes += retval; /* Save value. */ diff --git a/src/networking.c b/src/networking.c index e1b9ba04..33ed6c79 100644 --- a/src/networking.c +++ b/src/networking.c @@ -33,7 +33,7 @@ #include #include -static void setProtocolError(const char *errstr, client *c, int pos); +static void setProtocolError(const char *errstr, client *c, long pos); /* Return the size consumed from the allocator, for the specified SDS string, * including internal fragmentation. This function is used in order to compute @@ -1140,7 +1140,7 @@ int processInlineBuffer(client *c) { /* Helper function. Trims query buffer to make the function that processes * multi bulk requests idempotent. */ #define PROTO_DUMP_LEN 128 -static void setProtocolError(const char *errstr, client *c, int pos) { +static void setProtocolError(const char *errstr, client *c, long pos) { if (server.verbosity <= LL_VERBOSE) { sds client = catClientInfoString(sdsempty(),c); @@ -1181,7 +1181,8 @@ static void setProtocolError(const char *errstr, client *c, int pos) { * to be '*'. Otherwise for inline commands processInlineBuffer() is called. */ int processMultibulkBuffer(client *c) { char *newline = NULL; - int pos = 0, ok; + long pos = 0; + int ok; long long ll; if (c->multibulklen == 0) { @@ -1279,7 +1280,7 @@ int processMultibulkBuffer(client *c) { } /* Read bulk argument */ - if (sdslen(c->querybuf)-pos < (unsigned)(c->bulklen+2)) { + if (sdslen(c->querybuf)-pos < (size_t)(c->bulklen+2)) { /* Not enough data (+2 == trailing \r\n) */ break; } else { @@ -1288,7 +1289,7 @@ int processMultibulkBuffer(client *c) { * just use the current sds string. */ if (pos == 0 && c->bulklen >= PROTO_MBULK_BIG_ARG && - (signed) sdslen(c->querybuf) == c->bulklen+2) + sdslen(c->querybuf) == (size_t)(c->bulklen+2)) { c->argv[c->argc++] = createObject(OBJ_STRING,c->querybuf); sdsIncrLen(c->querybuf,-2); /* remove CRLF */ @@ -1399,7 +1400,7 @@ void readQueryFromClient(aeEventLoop *el, int fd, void *privdata, int mask) { if (c->reqtype == PROTO_REQ_MULTIBULK && c->multibulklen && c->bulklen != -1 && c->bulklen >= PROTO_MBULK_BIG_ARG) { - int remaining = (unsigned)(c->bulklen+2)-sdslen(c->querybuf); + ssize_t remaining = (size_t)(c->bulklen+2)-sdslen(c->querybuf); if (remaining < readlen) readlen = remaining; } diff --git a/src/rdb.c b/src/rdb.c index 28985b2a..5b7e2583 100644 --- a/src/rdb.c +++ b/src/rdb.c @@ -425,7 +425,7 @@ ssize_t rdbSaveLongLongAsStringObject(rio *rdb, long long value) { } /* Like rdbSaveRawString() gets a Redis object instead. */ -int rdbSaveStringObject(rio *rdb, robj *obj) { +ssize_t rdbSaveStringObject(rio *rdb, robj *obj) { /* Avoid to decode the object, then encode it again, if the * object is already integer encoded. */ if (obj->encoding == OBJ_ENCODING_INT) { @@ -861,21 +861,25 @@ int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, } /* Save an AUX field. */ -int rdbSaveAuxField(rio *rdb, void *key, size_t keylen, void *val, size_t vallen) { - if (rdbSaveType(rdb,RDB_OPCODE_AUX) == -1) return -1; - if (rdbSaveRawString(rdb,key,keylen) == -1) return -1; - if (rdbSaveRawString(rdb,val,vallen) == -1) return -1; - return 1; +ssize_t rdbSaveAuxField(rio *rdb, void *key, size_t keylen, void *val, size_t vallen) { + ssize_t ret, len = 0; + if ((ret = rdbSaveType(rdb,RDB_OPCODE_AUX)) == -1) return -1; + len += ret; + if ((ret = rdbSaveRawString(rdb,key,keylen) == -1)) return -1; + len += ret; + if ((ret = rdbSaveRawString(rdb,val,vallen) == -1)) return -1; + len += ret; + return len; } /* Wrapper for rdbSaveAuxField() used when key/val length can be obtained * with strlen(). */ -int rdbSaveAuxFieldStrStr(rio *rdb, char *key, char *val) { +ssize_t rdbSaveAuxFieldStrStr(rio *rdb, char *key, char *val) { return rdbSaveAuxField(rdb,key,strlen(key),val,strlen(val)); } /* Wrapper for strlen(key) + integer type (up to long long range). */ -int rdbSaveAuxFieldStrInt(rio *rdb, char *key, long long val) { +ssize_t rdbSaveAuxFieldStrInt(rio *rdb, char *key, long long val) { char buf[LONG_STR_SIZE]; int vlen = ll2string(buf,sizeof(buf),val); return rdbSaveAuxField(rdb,key,strlen(key),buf,vlen); diff --git a/src/rdb.h b/src/rdb.h index ecb066fb..2456cfb5 100644 --- a/src/rdb.h +++ b/src/rdb.h @@ -141,7 +141,7 @@ robj *rdbLoadObject(int type, rio *rdb); void backgroundSaveDoneHandler(int exitcode, int bysignal); int rdbSaveKeyValuePair(rio *rdb, robj *key, robj *val, long long expiretime, long long now); robj *rdbLoadStringObject(rio *rdb); -int rdbSaveStringObject(rio *rdb, robj *obj); +ssize_t rdbSaveStringObject(rio *rdb, robj *obj); ssize_t rdbSaveRawString(rio *rdb, unsigned char *s, size_t len); void *rdbGenericLoadStringObject(rio *rdb, int flags, size_t *lenptr); int rdbSaveBinaryDoubleValue(rio *rdb, double val); diff --git a/src/rio.c b/src/rio.c index 9c7220fc..23b90706 100644 --- a/src/rio.c +++ b/src/rio.c @@ -310,7 +310,7 @@ void rioSetAutoSync(rio *r, off_t bytes) { * generating the Redis protocol for the Append Only File. */ /* Write multi bulk count in the format: "*\r\n". */ -size_t rioWriteBulkCount(rio *r, char prefix, int count) { +size_t rioWriteBulkCount(rio *r, char prefix, long count) { char cbuf[128]; int clen; diff --git a/src/rio.h b/src/rio.h index 6749723d..c996c54f 100644 --- a/src/rio.h +++ b/src/rio.h @@ -130,7 +130,7 @@ void rioInitWithFdset(rio *r, int *fds, int numfds); void rioFreeFdset(rio *r); -size_t rioWriteBulkCount(rio *r, char prefix, int count); +size_t rioWriteBulkCount(rio *r, char prefix, long count); size_t rioWriteBulkString(rio *r, const char *buf, size_t len); size_t rioWriteBulkLongLong(rio *r, long long l); size_t rioWriteBulkDouble(rio *r, double d); diff --git a/src/sds.c b/src/sds.c index ff633c8b..603e36aa 100644 --- a/src/sds.c +++ b/src/sds.c @@ -175,7 +175,7 @@ void sdsfree(sds s) { * the output will be "6" as the string was modified but the logical length * remains 6 bytes. */ void sdsupdatelen(sds s) { - int reallen = strlen(s); + size_t reallen = strlen(s); sdssetlen(s, reallen); } @@ -319,7 +319,7 @@ void *sdsAllocPtr(sds s) { * ... check for nread <= 0 and handle it ... * sdsIncrLen(s, nread); */ -void sdsIncrLen(sds s, int incr) { +void sdsIncrLen(sds s, ssize_t incr) { unsigned char flags = s[-1]; size_t len; switch(flags&SDS_TYPE_MASK) { @@ -589,7 +589,7 @@ sds sdscatprintf(sds s, const char *fmt, ...) { sds sdscatfmt(sds s, char const *fmt, ...) { size_t initlen = sdslen(s); const char *f = fmt; - int i; + long i; va_list ap; va_start(ap,fmt); @@ -721,7 +721,7 @@ sds sdstrim(sds s, const char *cset) { * s = sdsnew("Hello World"); * sdsrange(s,1,-1); => "ello World" */ -void sdsrange(sds s, int start, int end) { +void sdsrange(sds s, ssize_t start, ssize_t end) { size_t newlen, len = sdslen(s); if (len == 0) return; @@ -735,9 +735,9 @@ void sdsrange(sds s, int start, int end) { } newlen = (start > end) ? 0 : (end-start)+1; if (newlen != 0) { - if (start >= (signed)len) { + if (start >= (ssize_t)len) { newlen = 0; - } else if (end >= (signed)len) { + } else if (end >= (ssize_t)len) { end = len-1; newlen = (start > end) ? 0 : (end-start)+1; } @@ -751,14 +751,14 @@ void sdsrange(sds s, int start, int end) { /* Apply tolower() to every character of the sds string 's'. */ void sdstolower(sds s) { - int len = sdslen(s), j; + size_t len = sdslen(s), j; for (j = 0; j < len; j++) s[j] = tolower(s[j]); } /* Apply toupper() to every character of the sds string 's'. */ void sdstoupper(sds s) { - int len = sdslen(s), j; + size_t len = sdslen(s), j; for (j = 0; j < len; j++) s[j] = toupper(s[j]); } @@ -782,7 +782,7 @@ int sdscmp(const sds s1, const sds s2) { l2 = sdslen(s2); minlen = (l1 < l2) ? l1 : l2; cmp = memcmp(s1,s2,minlen); - if (cmp == 0) return l1-l2; + if (cmp == 0) return l1>l2? 1: (l1