From 6448f735d5663c7c58aa269d8f53f06c4640ef5a Mon Sep 17 00:00:00 2001 From: Michael Grunder Date: Sun, 7 Jun 2020 14:38:16 -0700 Subject: [PATCH] sdsrange overflow fix (#830) Fix overflow bug in `sdsrange` --- hiredis.c | 6 +++--- hiredis.h | 5 +++-- net.c | 8 ++++---- net.h | 4 ++-- read.c | 2 +- sds.c | 15 +++++++++++---- sds.h | 4 +++- sockcompat.h | 2 +- ssl.c | 4 ++-- 9 files changed, 30 insertions(+), 20 deletions(-) diff --git a/hiredis.c b/hiredis.c index 3133716..4487134 100644 --- a/hiredis.c +++ b/hiredis.c @@ -920,17 +920,17 @@ int redisBufferWrite(redisContext *c, int *done) { return REDIS_ERR; if (sdslen(c->obuf) > 0) { - int nwritten = c->funcs->write(c); + ssize_t nwritten = c->funcs->write(c); if (nwritten < 0) { return REDIS_ERR; } else if (nwritten > 0) { - if (nwritten == (signed)sdslen(c->obuf)) { + if (nwritten == (ssize_t)sdslen(c->obuf)) { sdsfree(c->obuf); c->obuf = sdsempty(); if (c->obuf == NULL) goto oom; } else { - sdsrange(c->obuf,nwritten,-1); + if (sdsrange(c->obuf,nwritten,-1) < 0) goto oom; } } } diff --git a/hiredis.h b/hiredis.h index 9282671..bf10509 100644 --- a/hiredis.h +++ b/hiredis.h @@ -39,6 +39,7 @@ #include /* for struct timeval */ #else struct timeval; /* forward declaration */ +typedef long long ssize_t; #endif #include /* uintXX_t, etc */ #include "sds.h" /* for sds */ @@ -200,8 +201,8 @@ typedef struct redisContextFuncs { void (*free_privdata)(void *); void (*async_read)(struct redisAsyncContext *); void (*async_write)(struct redisAsyncContext *); - int (*read)(struct redisContext *, char *, size_t); - int (*write)(struct redisContext *); + ssize_t (*read)(struct redisContext *, char *, size_t); + ssize_t (*write)(struct redisContext *); } redisContextFuncs; /* Context for a connection to Redis */ diff --git a/net.c b/net.c index 54d904d..aa5b183 100644 --- a/net.c +++ b/net.c @@ -57,8 +57,8 @@ void redisNetClose(redisContext *c) { } } -int redisNetRead(redisContext *c, char *buf, size_t bufcap) { - int nread = recv(c->fd, buf, bufcap, 0); +ssize_t redisNetRead(redisContext *c, char *buf, size_t bufcap) { + ssize_t nread = recv(c->fd, buf, bufcap, 0); if (nread == -1) { if ((errno == EWOULDBLOCK && !(c->flags & REDIS_BLOCK)) || (errno == EINTR)) { /* Try again later */ @@ -79,8 +79,8 @@ int redisNetRead(redisContext *c, char *buf, size_t bufcap) { } } -int redisNetWrite(redisContext *c) { - int nwritten = send(c->fd, c->obuf, sdslen(c->obuf), 0); +ssize_t redisNetWrite(redisContext *c) { + ssize_t nwritten = send(c->fd, c->obuf, sdslen(c->obuf), 0); if (nwritten < 0) { if ((errno == EWOULDBLOCK && !(c->flags & REDIS_BLOCK)) || (errno == EINTR)) { /* Try again later */ diff --git a/net.h b/net.h index a4393c0..e28c403 100644 --- a/net.h +++ b/net.h @@ -38,8 +38,8 @@ #include "hiredis.h" void redisNetClose(redisContext *c); -int redisNetRead(redisContext *c, char *buf, size_t bufcap); -int redisNetWrite(redisContext *c); +ssize_t redisNetRead(redisContext *c, char *buf, size_t bufcap); +ssize_t redisNetWrite(redisContext *c); int redisCheckSocketError(redisContext *c); int redisContextSetTimeout(redisContext *c, const struct timeval tv); diff --git a/read.c b/read.c index d10b62a..544626e 100644 --- a/read.c +++ b/read.c @@ -720,7 +720,7 @@ int redisReaderGetReply(redisReader *r, void **reply) { /* Discard part of the buffer when we've consumed at least 1k, to avoid * doing unnecessary calls to memmove() in sds.c. */ if (r->pos >= 1024) { - sdsrange(r->buf,r->pos,-1); + if (sdsrange(r->buf,r->pos,-1) < 0) return REDIS_ERR; r->pos = 0; r->len = sdslen(r->buf); } diff --git a/sds.c b/sds.c index f7811a7..49d2096 100644 --- a/sds.c +++ b/sds.c @@ -36,6 +36,7 @@ #include #include #include +#include #include "sds.h" #include "sdsalloc.h" @@ -713,15 +714,20 @@ sds sdstrim(sds s, const char *cset) { * * The string is modified in-place. * + * Return value: + * -1 (error) if sdslen(s) is larger than maximum positive ssize_t value. + * 0 on success. + * * Example: * * s = sdsnew("Hello World"); * sdsrange(s,1,-1); => "ello World" */ -void sdsrange(sds s, int start, int end) { +int sdsrange(sds s, ssize_t start, ssize_t end) { size_t newlen, len = sdslen(s); + if (len > SSIZE_MAX) return -1; - if (len == 0) return; + if (len == 0) return 0; if (start < 0) { start = len+start; if (start < 0) start = 0; @@ -732,9 +738,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; } @@ -744,6 +750,7 @@ void sdsrange(sds s, int start, int end) { if (start && newlen) memmove(s, s+start, newlen); s[newlen] = 0; sdssetlen(s,newlen); + return 0; } /* Apply tolower() to every character of the sds string 's'. */ diff --git a/sds.h b/sds.h index 3f9a964..eda8833 100644 --- a/sds.h +++ b/sds.h @@ -36,6 +36,8 @@ #define SDS_MAX_PREALLOC (1024*1024) #ifdef _MSC_VER #define __attribute__(x) +typedef long long ssize_t; +#define SSIZE_MAX (LLONG_MAX >> 1) #endif #include @@ -239,7 +241,7 @@ sds sdscatprintf(sds s, const char *fmt, ...); sds sdscatfmt(sds s, char const *fmt, ...); sds sdstrim(sds s, const char *cset); -void sdsrange(sds s, int start, int end); +int sdsrange(sds s, ssize_t start, ssize_t end); void sdsupdatelen(sds s); void sdsclear(sds s); int sdscmp(const sds s1, const sds s2); diff --git a/sockcompat.h b/sockcompat.h index 56006c1..9249d5e 100644 --- a/sockcompat.h +++ b/sockcompat.h @@ -51,7 +51,7 @@ #include #ifdef _MSC_VER -typedef signed long ssize_t; +typedef long long ssize_t; #endif /* Emulate the parts of the BSD socket API that we need (override the winsock signatures). */ diff --git a/ssl.c b/ssl.c index f3870d5..10cf607 100644 --- a/ssl.c +++ b/ssl.c @@ -392,7 +392,7 @@ static void redisSSLFree(void *privdata){ hi_free(rsc); } -static int redisSSLRead(redisContext *c, char *buf, size_t bufcap) { +static ssize_t redisSSLRead(redisContext *c, char *buf, size_t bufcap) { redisSSL *rssl = c->privdata; int nread = SSL_read(rssl->ssl, buf, bufcap); @@ -434,7 +434,7 @@ static int redisSSLRead(redisContext *c, char *buf, size_t bufcap) { } } -static int redisSSLWrite(redisContext *c) { +static ssize_t redisSSLWrite(redisContext *c) { redisSSL *rssl = c->privdata; size_t len = rssl->lastLen ? rssl->lastLen : sdslen(c->obuf);