diff --git a/src/sds.c b/src/sds.c index d957ef4bab..6793c46caa 100644 --- a/src/sds.c +++ b/src/sds.c @@ -55,10 +55,10 @@ static inline int sdsHdrSize(char type) { static inline char sdsReqType(size_t string_size) { if (string_size < 1 << 5) return SDS_TYPE_5; - if (string_size < 1 << 8) return SDS_TYPE_8; - if (string_size < 1 << 16) return SDS_TYPE_16; + if (string_size <= (1 << 8) - sizeof(struct sdshdr8) - 1) return SDS_TYPE_8; + if (string_size <= (1 << 16) - sizeof(struct sdshdr16) - 1) return SDS_TYPE_16; #if (LONG_MAX == LLONG_MAX) - if (string_size < 1ll << 32) return SDS_TYPE_32; + if (string_size <= (1ll << 32) - sizeof(struct sdshdr32) - 1) return SDS_TYPE_32; return SDS_TYPE_64; #else return SDS_TYPE_32; @@ -75,6 +75,16 @@ static inline size_t sdsTypeMaxSize(char type) { return -1; /* this is equivalent to the max SDS_TYPE_64 or SDS_TYPE_32 */ } +static inline int adjustTypeIfNeeded(char *type, int *hdrlen, size_t bufsize) { + size_t usable = bufsize - *hdrlen - 1; + if (*type != SDS_TYPE_5 && usable > sdsTypeMaxSize(*type)) { + *type = sdsReqType(usable); + *hdrlen = sdsHdrSize(*type); + return 1; + } + return 0; +} + /* Create a new sds string with the content specified by the 'init' pointer * and 'initlen'. * If NULL is used for 'init' the string is initialized with zero bytes. @@ -97,19 +107,23 @@ sds _sdsnewlen(const void *init, size_t initlen, int trymalloc) { if (type == SDS_TYPE_5 && initlen == 0) type = SDS_TYPE_8; int hdrlen = sdsHdrSize(type); unsigned char *fp; /* flags pointer. */ - size_t usable; + size_t bufsize, usable; assert(initlen + hdrlen + 1 > initlen); /* Catch size_t overflow */ - sh = trymalloc ? s_trymalloc_usable(hdrlen + initlen + 1, &usable) : s_malloc_usable(hdrlen + initlen + 1, &usable); + sh = trymalloc ? s_trymalloc_usable(hdrlen + initlen + 1, &bufsize) + : s_malloc_usable(hdrlen + initlen + 1, &bufsize); if (sh == NULL) return NULL; if (init == SDS_NOINIT) init = NULL; else if (!init) memset(sh, 0, hdrlen + initlen + 1); + + adjustTypeIfNeeded(&type, &hdrlen, bufsize); + usable = bufsize - hdrlen - 1; + s = (char *)sh + hdrlen; fp = ((unsigned char *)s) - 1; - usable = usable - hdrlen - 1; - if (usable > sdsTypeMaxSize(type)) usable = sdsTypeMaxSize(type); + switch (type) { case SDS_TYPE_5: { *fp = type | (initlen << SDS_TYPE_BITS); @@ -118,6 +132,7 @@ sds _sdsnewlen(const void *init, size_t initlen, int trymalloc) { case SDS_TYPE_8: { SDS_HDR_VAR(8, s); sh->len = initlen; + assert(usable <= sdsTypeMaxSize(type)); sh->alloc = usable; *fp = type; break; @@ -125,6 +140,7 @@ sds _sdsnewlen(const void *init, size_t initlen, int trymalloc) { case SDS_TYPE_16: { SDS_HDR_VAR(16, s); sh->len = initlen; + assert(usable <= sdsTypeMaxSize(type)); sh->alloc = usable; *fp = type; break; @@ -132,6 +148,7 @@ sds _sdsnewlen(const void *init, size_t initlen, int trymalloc) { case SDS_TYPE_32: { SDS_HDR_VAR(32, s); sh->len = initlen; + assert(usable <= sdsTypeMaxSize(type)); sh->alloc = usable; *fp = type; break; @@ -139,6 +156,7 @@ sds _sdsnewlen(const void *init, size_t initlen, int trymalloc) { case SDS_TYPE_64: { SDS_HDR_VAR(64, s); sh->len = initlen; + assert(usable <= sdsTypeMaxSize(type)); sh->alloc = usable; *fp = type; break; @@ -226,7 +244,8 @@ sds _sdsMakeRoomFor(sds s, size_t addlen, int greedy) { size_t len, newlen, reqlen; char type, oldtype = s[-1] & SDS_TYPE_MASK; int hdrlen; - size_t usable; + size_t bufsize, usable; + int use_realloc; /* Return ASAP if there is enough space left. */ if (avail >= addlen) return s; @@ -251,23 +270,32 @@ sds _sdsMakeRoomFor(sds s, size_t addlen, int greedy) { hdrlen = sdsHdrSize(type); assert(hdrlen + newlen + 1 > reqlen); /* Catch size_t overflow */ - if (oldtype == type) { - newsh = s_realloc_usable(sh, hdrlen + newlen + 1, &usable); + use_realloc = (oldtype == type); + if (use_realloc) { + newsh = s_realloc_usable(sh, hdrlen + newlen + 1, &bufsize); if (newsh == NULL) return NULL; s = (char *)newsh + hdrlen; + + if (adjustTypeIfNeeded(&type, &hdrlen, bufsize)) { + memmove((char *)newsh + hdrlen, s, len + 1); + s = (char *)newsh + hdrlen; + s[-1] = type; + sdssetlen(s, len); + } } else { /* Since the header size changes, need to move the string forward, * and can't use realloc */ - newsh = s_malloc_usable(hdrlen + newlen + 1, &usable); + newsh = s_malloc_usable(hdrlen + newlen + 1, &bufsize); if (newsh == NULL) return NULL; + adjustTypeIfNeeded(&type, &hdrlen, bufsize); memcpy((char *)newsh + hdrlen, s, len + 1); s_free(sh); s = (char *)newsh + hdrlen; s[-1] = type; sdssetlen(s, len); } - usable = usable - hdrlen - 1; - if (usable > sdsTypeMaxSize(type)) usable = sdsTypeMaxSize(type); + usable = bufsize - hdrlen - 1; + assert(type == SDS_TYPE_5 || usable <= sdsTypeMaxSize(type)); sdssetalloc(s, usable); return s; } @@ -303,7 +331,7 @@ sds sdsRemoveFreeSpace(sds s, int would_regrow) { * allocation size, this is done in order to avoid repeated calls to this * function when the caller detects that it has excess space. */ sds sdsResize(sds s, size_t size, int would_regrow) { - void *sh, *newsh; + void *sh, *newsh = NULL; char type, oldtype = s[-1] & SDS_TYPE_MASK; int hdrlen, oldhdrlen = sdsHdrSize(oldtype); size_t len = sdslen(s); @@ -331,7 +359,8 @@ sds sdsResize(sds s, size_t size, int would_regrow) { * type. */ int use_realloc = (oldtype == type || (type < oldtype && type > SDS_TYPE_8)); size_t newlen = use_realloc ? oldhdrlen + size + 1 : hdrlen + size + 1; - size_t newsize = 0; + size_t bufsize = 0; + size_t newsize; if (use_realloc) { int alloc_already_optimal = 0; @@ -340,27 +369,37 @@ sds sdsResize(sds s, size_t size, int would_regrow) { * We aim to avoid calling realloc() when using Jemalloc if there is no * change in the allocation size, as it incurs a cost even if the * allocation size stays the same. */ - newsize = zmalloc_size(sh); - alloc_already_optimal = (je_nallocx(newlen, 0) == newsize); + bufsize = zmalloc_size(sh); + alloc_already_optimal = (je_nallocx(newlen, 0) == bufsize); #endif if (!alloc_already_optimal) { - newsh = s_realloc_usable(sh, newlen, &newsize); + newsh = s_realloc_usable(sh, newlen, &bufsize); if (newsh == NULL) return NULL; s = (char *)newsh + oldhdrlen; - newsize -= (oldhdrlen + 1); + + if (adjustTypeIfNeeded(&oldtype, &oldhdrlen, bufsize)) { + memmove((char *)newsh + oldhdrlen, s, len + 1); + s = (char *)newsh + oldhdrlen; + s[-1] = oldtype; + sdssetlen(s, len); + } } + newsize = bufsize - oldhdrlen - 1; + assert(oldtype == SDS_TYPE_5 || newsize <= sdsTypeMaxSize(oldtype)); } else { - newsh = s_malloc_usable(newlen, &newsize); + newsh = s_malloc_usable(newlen, &bufsize); if (newsh == NULL) return NULL; - memcpy((char *)newsh + hdrlen, s, len); + adjustTypeIfNeeded(&type, &hdrlen, bufsize); + memcpy((char *)newsh + hdrlen, s, len + 1); s_free(sh); s = (char *)newsh + hdrlen; s[-1] = type; - newsize -= (hdrlen + 1); + newsize = bufsize - hdrlen - 1; + assert(type == SDS_TYPE_5 || newsize <= sdsTypeMaxSize(type)); } + s[len] = '\0'; sdssetlen(s, len); - if (newsize > sdsTypeMaxSize(s[-1])) newsize = sdsTypeMaxSize(s[-1]); sdssetalloc(s, newsize); return s; } diff --git a/src/sdsalloc.h b/src/sdsalloc.h index 1252c7af81..6644eb3c83 100644 --- a/src/sdsalloc.h +++ b/src/sdsalloc.h @@ -49,5 +49,6 @@ #define s_realloc_usable zrealloc_usable #define s_trymalloc_usable ztrymalloc_usable #define s_tryrealloc_usable ztryrealloc_usable +#define s_malloc_size zmalloc_size #endif diff --git a/src/unit/test_files.h b/src/unit/test_files.h index 4a67f67052..a087e6fe44 100644 --- a/src/unit/test_files.h +++ b/src/unit/test_files.h @@ -24,6 +24,8 @@ int test_kvstoreIteratorRemoveAllKeysDeleteEmptyDict(int argc, char **argv, int int test_kvstoreDictIteratorRemoveAllKeysNoDeleteEmptyDict(int argc, char **argv, int flags); int test_kvstoreDictIteratorRemoveAllKeysDeleteEmptyDict(int argc, char **argv, int flags); int test_sds(int argc, char **argv, int flags); +int test_typesAndAllocSize(int argc, char **argv, int flags); +int test_sdsHeaderSizes(int argc, char **argv, int flags); int test_sha1(int argc, char **argv, int flags); int test_string2ll(int argc, char **argv, int flags); int test_string2l(int argc, char **argv, int flags); @@ -77,7 +79,7 @@ unitTest __test_crc64combine_c[] = {{"test_crc64combine", test_crc64combine}, {N unitTest __test_endianconv_c[] = {{"test_endianconv", test_endianconv}, {NULL, NULL}}; unitTest __test_intset_c[] = {{"test_intsetValueEncodings", test_intsetValueEncodings}, {"test_intsetBasicAdding", test_intsetBasicAdding}, {"test_intsetLargeNumberRandomAdd", test_intsetLargeNumberRandomAdd}, {"test_intsetUpgradeFromint16Toint32", test_intsetUpgradeFromint16Toint32}, {"test_intsetUpgradeFromint16Toint64", test_intsetUpgradeFromint16Toint64}, {"test_intsetUpgradeFromint32Toint64", test_intsetUpgradeFromint32Toint64}, {"test_intsetStressLookups", test_intsetStressLookups}, {"test_intsetStressAddDelete", test_intsetStressAddDelete}, {NULL, NULL}}; unitTest __test_kvstore_c[] = {{"test_kvstoreAdd16Keys", test_kvstoreAdd16Keys}, {"test_kvstoreIteratorRemoveAllKeysNoDeleteEmptyDict", test_kvstoreIteratorRemoveAllKeysNoDeleteEmptyDict}, {"test_kvstoreIteratorRemoveAllKeysDeleteEmptyDict", test_kvstoreIteratorRemoveAllKeysDeleteEmptyDict}, {"test_kvstoreDictIteratorRemoveAllKeysNoDeleteEmptyDict", test_kvstoreDictIteratorRemoveAllKeysNoDeleteEmptyDict}, {"test_kvstoreDictIteratorRemoveAllKeysDeleteEmptyDict", test_kvstoreDictIteratorRemoveAllKeysDeleteEmptyDict}, {NULL, NULL}}; -unitTest __test_sds_c[] = {{"test_sds", test_sds}, {NULL, NULL}}; +unitTest __test_sds_c[] = {{"test_sds", test_sds}, {"test_typesAndAllocSize", test_typesAndAllocSize}, {"test_sdsHeaderSizes", test_sdsHeaderSizes}, {NULL, NULL}}; unitTest __test_sha1_c[] = {{"test_sha1", test_sha1}, {NULL, NULL}}; unitTest __test_util_c[] = {{"test_string2ll", test_string2ll}, {"test_string2l", test_string2l}, {"test_ll2string", test_ll2string}, {"test_ld2string", test_ld2string}, {"test_fixedpoint_d2string", test_fixedpoint_d2string}, {"test_version2num", test_version2num}, {"test_reclaimFilePageCache", test_reclaimFilePageCache}, {NULL, NULL}}; unitTest __test_ziplist_c[] = {{"test_ziplistCreateIntList", test_ziplistCreateIntList}, {"test_ziplistPop", test_ziplistPop}, {"test_ziplistGetElementAtIndex3", test_ziplistGetElementAtIndex3}, {"test_ziplistGetElementOutOfRange", test_ziplistGetElementOutOfRange}, {"test_ziplistGetLastElement", test_ziplistGetLastElement}, {"test_ziplistGetFirstElement", test_ziplistGetFirstElement}, {"test_ziplistGetElementOutOfRangeReverse", test_ziplistGetElementOutOfRangeReverse}, {"test_ziplistIterateThroughFullList", test_ziplistIterateThroughFullList}, {"test_ziplistIterateThroughListFrom1ToEnd", test_ziplistIterateThroughListFrom1ToEnd}, {"test_ziplistIterateThroughListFrom2ToEnd", test_ziplistIterateThroughListFrom2ToEnd}, {"test_ziplistIterateThroughStartOutOfRange", test_ziplistIterateThroughStartOutOfRange}, {"test_ziplistIterateBackToFront", test_ziplistIterateBackToFront}, {"test_ziplistIterateBackToFrontDeletingAllItems", test_ziplistIterateBackToFrontDeletingAllItems}, {"test_ziplistDeleteInclusiveRange0To0", test_ziplistDeleteInclusiveRange0To0}, {"test_ziplistDeleteInclusiveRange0To1", test_ziplistDeleteInclusiveRange0To1}, {"test_ziplistDeleteInclusiveRange1To2", test_ziplistDeleteInclusiveRange1To2}, {"test_ziplistDeleteWithStartIndexOutOfRange", test_ziplistDeleteWithStartIndexOutOfRange}, {"test_ziplistDeleteWithNumOverflow", test_ziplistDeleteWithNumOverflow}, {"test_ziplistDeleteFooWhileIterating", test_ziplistDeleteFooWhileIterating}, {"test_ziplistReplaceWithSameSize", test_ziplistReplaceWithSameSize}, {"test_ziplistReplaceWithDifferentSize", test_ziplistReplaceWithDifferentSize}, {"test_ziplistRegressionTestForOver255ByteStrings", test_ziplistRegressionTestForOver255ByteStrings}, {"test_ziplistRegressionTestDeleteNextToLastEntries", test_ziplistRegressionTestDeleteNextToLastEntries}, {"test_ziplistCreateLongListAndCheckIndices", test_ziplistCreateLongListAndCheckIndices}, {"test_ziplistCompareStringWithZiplistEntries", test_ziplistCompareStringWithZiplistEntries}, {"test_ziplistMergeTest", test_ziplistMergeTest}, {"test_ziplistStressWithRandomPayloadsOfDifferentEncoding", test_ziplistStressWithRandomPayloadsOfDifferentEncoding}, {"test_ziplistCascadeUpdateEdgeCases", test_ziplistCascadeUpdateEdgeCases}, {"test_ziplistInsertEdgeCase", test_ziplistInsertEdgeCase}, {"test_ziplistStressWithVariableSize", test_ziplistStressWithVariableSize}, {"test_BenchmarkziplistFind", test_BenchmarkziplistFind}, {"test_BenchmarkziplistIndex", test_BenchmarkziplistIndex}, {"test_BenchmarkziplistValidateIntegrity", test_BenchmarkziplistValidateIntegrity}, {"test_BenchmarkziplistCompareWithString", test_BenchmarkziplistCompareWithString}, {"test_BenchmarkziplistCompareWithNumber", test_BenchmarkziplistCompareWithNumber}, {"test_ziplistStress__ziplistCascadeUpdate", test_ziplistStress__ziplistCascadeUpdate}, {NULL, NULL}}; diff --git a/src/unit/test_sds.c b/src/unit/test_sds.c index adf3d37f2c..19b5c7d73f 100644 --- a/src/unit/test_sds.c +++ b/src/unit/test_sds.c @@ -4,6 +4,7 @@ #include "test_help.h" #include "../sds.h" +#include "../sdsalloc.h" static sds sdsTestTemplateCallback(sds varname, void *arg) { UNUSED(arg); @@ -247,5 +248,83 @@ int test_sds(int argc, char **argv, int flags) { TEST_ASSERT_MESSAGE("sdsReszie() crop strlen", strlen(x) == 4); TEST_ASSERT_MESSAGE("sdsReszie() crop alloc", sdsalloc(x) >= 4); sdsfree(x); + + return 0; +} + +int test_typesAndAllocSize(int argc, char **argv, int flags) { + UNUSED(argc); + UNUSED(argv); + UNUSED(flags); + + sds x = sdsnewlen(NULL, 31); + TEST_ASSERT_MESSAGE("len 31 type", (x[-1] & SDS_TYPE_MASK) == SDS_TYPE_5); + sdsfree(x); + + x = sdsnewlen(NULL, 32); + TEST_ASSERT_MESSAGE("len 32 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_8); + TEST_ASSERT_MESSAGE("len 32 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x))); + sdsfree(x); + + x = sdsnewlen(NULL, 252); + TEST_ASSERT_MESSAGE("len 252 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_8); + TEST_ASSERT_MESSAGE("len 252 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x))); + sdsfree(x); + + x = sdsnewlen(NULL, 253); + TEST_ASSERT_MESSAGE("len 253 type", (x[-1] & SDS_TYPE_MASK) == SDS_TYPE_16); + TEST_ASSERT_MESSAGE("len 253 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x))); + sdsfree(x); + + x = sdsnewlen(NULL, 65530); + TEST_ASSERT_MESSAGE("len 65530 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_16); + TEST_ASSERT_MESSAGE("len 65530 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x))); + sdsfree(x); + + x = sdsnewlen(NULL, 65531); + TEST_ASSERT_MESSAGE("len 65531 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_32); + TEST_ASSERT_MESSAGE("len 65531 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x))); + sdsfree(x); + +#if (LONG_MAX == LLONG_MAX) + if (flags & UNIT_TEST_LARGE_MEMORY) { + x = sdsnewlen(NULL, 4294967286); + TEST_ASSERT_MESSAGE("len 4294967286 type", (x[-1] & SDS_TYPE_MASK) >= SDS_TYPE_32); + TEST_ASSERT_MESSAGE("len 4294967286 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x))); + sdsfree(x); + + x = sdsnewlen(NULL, 4294967287); + TEST_ASSERT_MESSAGE("len 4294967287 type", (x[-1] & SDS_TYPE_MASK) == SDS_TYPE_64); + TEST_ASSERT_MESSAGE("len 4294967287 sdsAllocSize", sdsAllocSize(x) == s_malloc_size(sdsAllocPtr(x))); + sdsfree(x); + } +#endif + + return 0; +} + +/* The test verifies that we can adjust SDS types if an allocator returned + * larger buffer. The maximum length for type SDS_TYPE_X is + * 2^X - header_size(SDS_TYPE_X) - 1. The maximum value to be stored in alloc + * field is 2^X - 1. When allocated buffer is larger than + * 2^X + header_size(SDS_TYPE_X), we "move" to a larger type SDS_TYPE_Y. To be + * sure SDS_TYPE_Y header fits into 2^X + header_size(SDS_TYPE_X) + 1 bytes, the + * difference between header sizes must be smaller than + * header_size(SDS_TYPE_X) + 1. + * We ignore SDS_TYPE_5 as it doesn't have alloc field. */ +int test_sdsHeaderSizes(int argc, char **argv, int flags) { + UNUSED(argc); + UNUSED(argv); + UNUSED(flags); + + TEST_ASSERT_MESSAGE("can't always adjust SDS_TYPE_8 with SDS_TYPE_16", + sizeof(struct sdshdr16) <= 2 * sizeof(struct sdshdr8) + 1); + TEST_ASSERT_MESSAGE("can't always adjust SDS_TYPE_16 with SDS_TYPE_32", + sizeof(struct sdshdr32) <= 2 * sizeof(struct sdshdr16) + 1); +#if (LONG_MAX == LLONG_MAX) + TEST_ASSERT_MESSAGE("can't always adjust SDS_TYPE_32 with SDS_TYPE_64", + sizeof(struct sdshdr64) <= 2 * sizeof(struct sdshdr32) + 1); +#endif + return 0; }