summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorOzan Tezcan <ozantezcan@gmail.com>2021-11-11 14:51:33 +0300
committerOran Agra <oran@redislabs.com>2023-01-17 14:59:41 +0200
commitcb6beec2e3576442d16c045ecd9aca5f2a4a911a (patch)
treefbe62082e76fe40c3fd95e34e48d236909c56fb9
parent542ccdc9168c6eb792af6e57d261d8d8b1b33a33 (diff)
downloadredis-cb6beec2e3576442d16c045ecd9aca5f2a4a911a.tar.gz
Some fixes to undefined behaviour bugs taken from (#9601)
**Signed integer overflow.** Although, signed overflow issue can be problematic time to time and change how compiler generates code, current findings mostly about signed shift or simple addition overflow. For most platforms Redis can be compiled for, this wouldn't cause any issue as far as I can tell (checked generated code on godbolt.org). UB means nothing guaranteed and risky to reason about program behavior but I don't think any of the fixes here worth backporting. As sanitizers are now part of the CI, preventing new issues will be the real benefit. partial cherry pick from commit b91d8b289bb64965c5eaa445809f9f49293e99c0 The bug in BITFIELD seems to affect 12.2.1 used on Alpine (cherry picked from commit 4418cf166e025e7d0d2c965e75ad57c05ecff43f)
-rw-r--r--src/bitops.c5
-rw-r--r--src/config.c2
-rw-r--r--src/dict.c4
-rw-r--r--src/dict.h2
-rw-r--r--src/rax.c1
-rw-r--r--src/rdb.c8
-rw-r--r--src/sha256.c9
-rw-r--r--src/ziplist.c10
8 files changed, 26 insertions, 15 deletions
diff --git a/src/bitops.c b/src/bitops.c
index c8862516b..5cad11f7d 100644
--- a/src/bitops.c
+++ b/src/bitops.c
@@ -328,8 +328,9 @@ int checkSignedBitfieldOverflow(int64_t value, int64_t incr, uint64_t bits, int
/* Note that maxincr and minincr could overflow, but we use the values
* only after checking 'value' range, so when we use it no overflow
- * happens. */
- int64_t maxincr = max-value;
+ * happens. 'uint64_t' cast is there just to prevent undefined behavior on
+ * overflow */
+ int64_t maxincr = (uint64_t)max-value;
int64_t minincr = min-value;
if (value > max || (bits != 64 && incr > maxincr) || (value >= 0 && incr > 0 && incr > maxincr))
diff --git a/src/config.c b/src/config.c
index 15ab7e8a4..03104f56c 100644
--- a/src/config.c
+++ b/src/config.c
@@ -363,6 +363,7 @@ void initConfigValues() {
void loadServerConfigFromString(char *config) {
char *err = NULL;
+ char buf[1024];
int linenum = 0, totlines, i;
int slaveof_linenum = 0;
sds *lines;
@@ -575,7 +576,6 @@ void loadServerConfigFromString(char *config) {
} else if (!strcasecmp(argv[0],"user") && argc >= 2) {
int argc_err;
if (ACLAppendUserForLoading(argv,argc,&argc_err) == C_ERR) {
- char buf[1024];
char *errmsg = ACLSetUserStringError();
snprintf(buf,sizeof(buf),"Error in user declaration '%s': %s",
argv[argc_err],errmsg);
diff --git a/src/dict.c b/src/dict.c
index 45a29397e..7abe9259b 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -509,8 +509,8 @@ void *dictFetchValue(dict *d, const void *key) {
* the fingerprint again when the iterator is released.
* If the two fingerprints are different it means that the user of the iterator
* performed forbidden operations against the dictionary while iterating. */
-long long dictFingerprint(dict *d) {
- long long integers[6], hash = 0;
+unsigned long long dictFingerprint(dict *d) {
+ unsigned long long integers[6], hash = 0;
int j;
integers[0] = (long) d->ht[0].table;
diff --git a/src/dict.h b/src/dict.h
index edcb4c23e..5507ee322 100644
--- a/src/dict.h
+++ b/src/dict.h
@@ -94,7 +94,7 @@ typedef struct dictIterator {
int table, safe;
dictEntry *entry, *nextEntry;
/* unsafe iterator fingerprint for misuse detection. */
- long long fingerprint;
+ unsigned long long fingerprint;
} dictIterator;
typedef void (dictScanFunction)(void *privdata, const dictEntry *de);
diff --git a/src/rax.c b/src/rax.c
index 5768071c0..1812c11d3 100644
--- a/src/rax.c
+++ b/src/rax.c
@@ -1270,6 +1270,7 @@ void raxStart(raxIterator *it, rax *rt) {
* is a low level function used to implement the iterator, not callable by
* the user. Returns 0 on out of memory, otherwise 1 is returned. */
int raxIteratorAddChars(raxIterator *it, unsigned char *s, size_t len) {
+ if (len == 0) return 1;
if (it->key_max < it->key_len+len) {
unsigned char *old = (it->key == it->key_static_string) ? NULL :
it->key;
diff --git a/src/rdb.c b/src/rdb.c
index 11cc41b56..1227bb34b 100644
--- a/src/rdb.c
+++ b/src/rdb.c
@@ -278,12 +278,16 @@ void *rdbLoadIntegerObject(rio *rdb, int enctype, int flags, size_t *lenptr) {
} else if (enctype == RDB_ENC_INT16) {
uint16_t v;
if (rioRead(rdb,enc,2) == 0) return NULL;
- v = enc[0]|(enc[1]<<8);
+ v = ((uint32_t)enc[0])|
+ ((uint32_t)enc[1]<<8);
val = (int16_t)v;
} else if (enctype == RDB_ENC_INT32) {
uint32_t v;
if (rioRead(rdb,enc,4) == 0) return NULL;
- v = enc[0]|(enc[1]<<8)|(enc[2]<<16)|(enc[3]<<24);
+ v = ((uint32_t)enc[0])|
+ ((uint32_t)enc[1]<<8)|
+ ((uint32_t)enc[2]<<16)|
+ ((uint32_t)enc[3]<<24);
val = (int32_t)v;
} else {
rdbExitReportCorruptRDB("Unknown RDB integer encoding type %d",enctype);
diff --git a/src/sha256.c b/src/sha256.c
index d644d2d4e..3b53c45c4 100644
--- a/src/sha256.c
+++ b/src/sha256.c
@@ -45,8 +45,13 @@ void sha256_transform(SHA256_CTX *ctx, const BYTE data[])
{
WORD a, b, c, d, e, f, g, h, i, j, t1, t2, m[64];
- for (i = 0, j = 0; i < 16; ++i, j += 4)
- m[i] = (data[j] << 24) | (data[j + 1] << 16) | (data[j + 2] << 8) | (data[j + 3]);
+ for (i = 0, j = 0; i < 16; ++i, j += 4) {
+ m[i] = ((WORD) data[j + 0] << 24) |
+ ((WORD) data[j + 1] << 16) |
+ ((WORD) data[j + 2] << 8) |
+ ((WORD) data[j + 3]);
+ }
+
for ( ; i < 64; ++i)
m[i] = SIG1(m[i - 2]) + m[i - 7] + SIG0(m[i - 15]) + m[i - 16];
diff --git a/src/ziplist.c b/src/ziplist.c
index 582eed2c9..6c13a4492 100644
--- a/src/ziplist.c
+++ b/src/ziplist.c
@@ -390,10 +390,10 @@ unsigned int zipStoreEntryEncoding(unsigned char *p, unsigned char encoding, uns
(len) = (((ptr)[0] & 0x3f) << 8) | (ptr)[1]; \
} else if ((encoding) == ZIP_STR_32B) { \
(lensize) = 5; \
- (len) = ((ptr)[1] << 24) | \
- ((ptr)[2] << 16) | \
- ((ptr)[3] << 8) | \
- ((ptr)[4]); \
+ (len) = ((uint32_t)(ptr)[1] << 24) | \
+ ((uint32_t)(ptr)[2] << 16) | \
+ ((uint32_t)(ptr)[3] << 8) | \
+ ((uint32_t)(ptr)[4]); \
} else { \
panic("Invalid string encoding 0x%02X", (encoding)); \
} \
@@ -526,7 +526,7 @@ void zipSaveInteger(unsigned char *p, int64_t value, unsigned char encoding) {
memcpy(p,&i16,sizeof(i16));
memrev16ifbe(p);
} else if (encoding == ZIP_INT_24B) {
- i32 = value<<8;
+ i32 = ((uint64_t)value)<<8;
memrev32ifbe(&i32);
memcpy(p,((uint8_t*)&i32)+1,sizeof(i32)-sizeof(uint8_t));
} else if (encoding == ZIP_INT_32B) {