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.

(cherry picked from commit ae418eca24ba53a7dca07b0e7065f856e625469b)

src/module.c

@@ -2538,19 +2538,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;

src/sds.c

@@ -759,6 +759,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.
  *
@@ -770,6 +785,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");
@@ -777,28 +797,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 = (start > end) ? 0 : (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'. */
@@ -1353,6 +1358,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");

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);

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);