diff options
author | Ozan Tezcan <ozantezcan@gmail.com> | 2021-11-11 14:51:33 +0300 |
---|---|---|
committer | Oran Agra <oran@redislabs.com> | 2023-01-17 14:59:41 +0200 |
commit | cb6beec2e3576442d16c045ecd9aca5f2a4a911a (patch) | |
tree | fbe62082e76fe40c3fd95e34e48d236909c56fb9 | |
parent | 542ccdc9168c6eb792af6e57d261d8d8b1b33a33 (diff) | |
download | redis-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.c | 5 | ||||
-rw-r--r-- | src/config.c | 2 | ||||
-rw-r--r-- | src/dict.c | 4 | ||||
-rw-r--r-- | src/dict.h | 2 | ||||
-rw-r--r-- | src/rax.c | 1 | ||||
-rw-r--r-- | src/rdb.c | 8 | ||||
-rw-r--r-- | src/sha256.c | 9 | ||||
-rw-r--r-- | src/ziplist.c | 10 |
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); @@ -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; @@ -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) { |