summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorOzan Tezcan <ozantezcan@gmail.com>2021-11-11 14:51:33 +0300
committerOran Agra <oran@redislabs.com>2022-12-12 17:02:54 +0200
commit052e01e75df982d929821cdd704cf5c870cc757d (patch)
tree1f5ced6e1a4ad7d40fc3665df6a83ec8c33b2143
parent5d66aa3d22fe74338bc96690d379cc7a5265f143 (diff)
downloadredis-052e01e75df982d929821cdd704cf5c870cc757d.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
-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 f1c563a41..841f6d644 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 e0eec2f67..691b8a045 100644
--- a/src/config.c
+++ b/src/config.c
@@ -388,6 +388,7 @@ void initConfigValues() {
}
void loadServerConfigFromString(char *config) {
+ char buf[1024];
const char *err = NULL;
int linenum = 0, totlines, i;
int slaveof_linenum = 0;
@@ -588,7 +589,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];
const 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 1bbdc211e..339e8c6c3 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -534,8 +534,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 7e2258960..3b7d46d71 100644
--- a/src/dict.h
+++ b/src/dict.h
@@ -95,7 +95,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 0826b974a..2e79aed7c 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 ff1fd294c..5cd85ac07 100644
--- a/src/rdb.c
+++ b/src/rdb.c
@@ -288,12 +288,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 {
rdbReportCorruptRDB("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 7a3263d8c..b58e6cb97 100644
--- a/src/ziplist.c
+++ b/src/ziplist.c
@@ -416,10 +416,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 { \
(lensize) = 0; /* bad encoding, should be covered by a previous */ \
(len) = 0; /* ZIP_ASSERT_ENCODING / zipEncodingLenSize, or */ \
@@ -557,7 +557,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) {