From ae418eca24ba53a7dca07b0e7065f856e625469b Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Tue, 22 Jun 2021 17:22:40 +0300 Subject: [PATCH] Adjustments to recent RM_StringTruncate fix (#3718) (#9125) - Introduce a new sdssubstr api as a building block for sdsrange. The API of sdsrange is many times hard to work with and also has corner case that cause bugs. sdsrange is easy to work with and also simplifies the implementation of sdsrange. - Revert the fix to RM_StringTruncate and just use sdssubstr instead of sdsrange. - Solve valgrind warnings from the new tests introduced by the previous PR. --- src/module.c | 21 ++++++---------- src/sds.c | 57 +++++++++++++++++++++++++++--------------- src/sds.h | 1 + tests/modules/basics.c | 3 +++ 4 files changed, 49 insertions(+), 33 deletions(-) diff --git a/src/module.c b/src/module.c index b7108cd6d..8fdbf2ccb 100644 --- a/src/module.c +++ b/src/module.c @@ -2559,19 +2559,14 @@ int RM_StringTruncate(RedisModuleKey *key, size_t newlen) { } else { /* Unshare and resize. */ key->value = dbUnshareStringValue(key->db, key->key, key->value); - if (newlen == 0) { - sdsfree(key->value->ptr); - key->value->ptr = sdsempty(); - } else { - size_t curlen = sdslen(key->value->ptr); - if (newlen > curlen) { - key->value->ptr = sdsgrowzero(key->value->ptr,newlen); - } else if (newlen < curlen) { - sdsrange(key->value->ptr,0,newlen-1); - /* If the string is too wasteful, reallocate it. */ - if (sdslen(key->value->ptr) < sdsavail(key->value->ptr)) - key->value->ptr = sdsRemoveFreeSpace(key->value->ptr); - } + size_t curlen = sdslen(key->value->ptr); + if (newlen > curlen) { + key->value->ptr = sdsgrowzero(key->value->ptr,newlen); + } else if (newlen < curlen) { + sdssubstr(key->value->ptr,0,newlen); + /* If the string is too wasteful, reallocate it. */ + if (sdslen(key->value->ptr) < sdsavail(key->value->ptr)) + key->value->ptr = sdsRemoveFreeSpace(key->value->ptr); } } return REDISMODULE_OK; diff --git a/src/sds.c b/src/sds.c index 31ab2ed3a..036d714d4 100644 --- a/src/sds.c +++ b/src/sds.c @@ -778,6 +778,21 @@ sds sdstrim(sds s, const char *cset) { return s; } +/* Changes the input string to be a subset of the original. + * It does not release the free space in the string, so a call to + * sdsRemoveFreeSpace may be wise after. */ +void sdssubstr(sds s, size_t start, size_t len) { + /* Clamp out of range input */ + size_t oldlen = sdslen(s); + if (start >= oldlen) start = len = 0; + if (len > oldlen-start) len = oldlen-start; + + /* Move the data */ + if (len) memmove(s, s+start, len); + s[len] = 0; + sdssetlen(s,len); +} + /* Turn the string into a smaller (or equal) string containing only the * substring specified by the 'start' and 'end' indexes. * @@ -789,6 +804,11 @@ sds sdstrim(sds s, const char *cset) { * * The string is modified in-place. * + * NOTE: this function can be misleading and can have unexpected behaviour, + * specifically when you want the length of the new string to be 0. + * Having start==end will result in a string with one character. + * please consider using sdssubstr instead. + * * Example: * * s = sdsnew("Hello World"); @@ -796,28 +816,13 @@ sds sdstrim(sds s, const char *cset) { */ void sdsrange(sds s, ssize_t start, ssize_t end) { size_t newlen, len = sdslen(s); - if (len == 0) return; - if (start < 0) { - start = len+start; - if (start < 0) start = 0; - } - if (end < 0) { - end = len+end; - if (end < 0) end = 0; - } + if (start < 0) + start = len + start; + if (end < 0) + end = len + end; newlen = (start > end) ? 0 : (end-start)+1; - if (newlen != 0) { - if (start >= (ssize_t)len) { - newlen = 0; - } else if (end >= (ssize_t)len) { - end = len-1; - newlen = (end-start)+1; - } - } - if (start && newlen) memmove(s, s+start, newlen); - s[newlen] = 0; - sdssetlen(s,newlen); + sdssubstr(s, start, newlen); } /* Apply tolower() to every character of the sds string 's'. */ @@ -1372,6 +1377,18 @@ int sdsTest(int argc, char **argv, int accurate) { test_cond("sdsrange(...,100,100)", sdslen(y) == 0 && memcmp(y,"\0",1) == 0); + sdsfree(y); + y = sdsdup(x); + sdsrange(y,4,6); + test_cond("sdsrange(...,4,6)", + sdslen(y) == 0 && memcmp(y,"\0",1) == 0); + + sdsfree(y); + y = sdsdup(x); + sdsrange(y,3,6); + test_cond("sdsrange(...,3,6)", + sdslen(y) == 1 && memcmp(y,"o\0",2) == 0); + sdsfree(y); sdsfree(x); x = sdsnew("foo"); diff --git a/src/sds.h b/src/sds.h index 8392c2064..9b01b4e4e 100644 --- a/src/sds.h +++ b/src/sds.h @@ -238,6 +238,7 @@ sds sdscatprintf(sds s, const char *fmt, ...); sds sdscatfmt(sds s, char const *fmt, ...); sds sdstrim(sds s, const char *cset); +void sdssubstr(sds s, size_t start, size_t len); void sdsrange(sds s, ssize_t start, ssize_t end); void sdsupdatelen(sds s); void sdsclear(sds s); diff --git a/tests/modules/basics.c b/tests/modules/basics.c index 9786be1b9..2618f45b8 100644 --- a/tests/modules/basics.c +++ b/tests/modules/basics.c @@ -156,6 +156,7 @@ int TestUnlink(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { /* TEST.STRING.TRUNCATE -- Test truncating an existing string object. */ int TestStringTruncate(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { + RedisModule_AutoMemory(ctx); REDISMODULE_NOT_USED(argv); REDISMODULE_NOT_USED(argc); @@ -208,6 +209,7 @@ int TestStringTruncate(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) int NotifyCallback(RedisModuleCtx *ctx, int type, const char *event, RedisModuleString *key) { + RedisModule_AutoMemory(ctx); /* Increment a counter on the notifications: for each key notified we * increment a counter */ RedisModule_Log(ctx, "notice", "Got event type %d, event %s, key %s", type, @@ -219,6 +221,7 @@ int NotifyCallback(RedisModuleCtx *ctx, int type, const char *event, /* TEST.NOTIFICATIONS -- Test Keyspace Notifications. */ int TestNotifications(RedisModuleCtx *ctx, RedisModuleString **argv, int argc) { + RedisModule_AutoMemory(ctx); REDISMODULE_NOT_USED(argv); REDISMODULE_NOT_USED(argc);