antirez / sds Goto Github PK

Hi Salvatore.

Thanks for releasing this. You should consider using the GCC (and compatible) attribute "warn_unused_result" on the functions that may call realloc().

http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bwarn_005funused_005fresult_007d-attribute-2997

It's not a perfect backstop, but it can be very helpful in calling attention to usage that would otherwise be a very subtle error.

When a new sbs is created (as a char array) it is casted to struct type pointer, ex. SDS_HDR(8,s);. Isn't this violating the strict aliasing rule?
(Dereferencing a pointer that aliases an object that is not of a compatible type or one of the other types allowed by C 2011 6.5 paragraph 71 is undefined behavior)

Example:

sh = s_malloc(hdrlen+initlen+1);
...
s = (char*)sh+hdrlen;
...
SDS_HDR_VAR(8,s);
sh->len = initlen;
sh->alloc = initlen;

#define SDS_HDR_VAR(T,s) struct sdshdr##T *sh = (void*)((s)-(sizeof(struct sdshdr##T)));
#define SDS_HDR(T,s) ((struct sdshdr##T *)((s)-(sizeof(struct sdshdr##T))))

Am I missing something or support for alternative allocators should be added?

Why are there so many pull requests, even really old ones without any response to say if they'll be incorporated or not, or if you don't yet have time to review them?

I think leaving those PRs open discourages possible developers from contributing, am I missing something? If so, I'm sorry, but I'm really curious.

The value written to &oldfree (sds.c, line 1240) is never used.

if (type != SDS_TYPE_5) {
         test_cond("sdsMakeRoomFor() free", sdsavail(x) >= step);
         oldfree = sdsavail(x);
}

One way to correct this error is to reverse the order of lines 1239 and 1240 and replace sdsavail (x)> = step with oldfree> = step.

https://github.com/antirez/sds/blob/master/sds.c#L46

I would like to point out that the identifier "__SDS_H" does not fit to the expected naming convention of the C language standard.
Would you like to adjust your selection for unique names?

In the function sdsIncrLen, I think the statement "assert(sh->free >= 0) " is redundant, because "assert(sh->free >= incr)" makes sure this situation would not happen.

void sdsIncrLen(sds s, int incr) {
struct sdshdr sh = (void)(s - sizeof *sh);
assert(sh->free >= incr);
sh->len += incr;
sh->free -= incr;
assert(sh->free >= 0);
s[sh->len] = '\0';
}

In the function 'sdscatvprintf' I think the statement va_end(cpy) is missing after vsnprintf call.

static inline size_t sdslen(const sds s) {
and
static inline size_t sdsavail(const sds s) {

google says to use __inline not inline

I like the API of SDS but I see no (development) activity in this repository. Is it still maintained?

The sds source in the redis seems to be newer?

I've been investigating a nasty firmware crash that, until now, was a huge mystery.

While debugging some new code that uses SDS, I single stepped into sdscmp(). There, I noticed that one of the two variables was totally different than the ones being compared. And, when sdscmp() returned, the stack pointer was apparently ruined, as I was taken back to a process that a 1-second interrupt handler uses for sdscmp().

I suppose it's possible this is an artifact of my debugger (Atmel Studio 7).

However, if not, could there be a case where the sdscmp() function (or for that matter any sds function) cannot be called by two different processes, one of which is an interrupt service routine?

I find that almost in every function, you write "struct sdshdr sh = (void)(s - sizeof *sh);" to get the address of sdshdr, I wonder why don't you make it macro?
Like this :
#define SDS_ADDR(s) (void *)((s) - sizeof(struct sdshdr))
struct sdshdr *sh = SDS_ADDR(s);

Thank you for a fantastic library!!

One minor possible issue:

I am using sdssplitlen() to parse out the elements of a path+file string, as in:
/DIR1/DIR2/this_isafile.txt <-- cTempString

tokens = sdssplitlen(cTempString,sdslen(cTempString),"/",1,&count);

With an underscore in the string, sdssplitlen() returns NULL.

Haven't single-step debugged it yet. For now, avoiding using filenames with _

How do you feel about publishing this to clibs? :]

Just add a package.json like:
https://github.com/jwerle/libutf8/blob/master/package.json

Then publish here:
https://github.com/clibs/clib/wiki/Packages

Google doesn't indicate any connection between sds and Arduino, Stm32, etc. And it seems that lightweight and comfortable sds is a welcomed proportion between string.h and C++ string and it could be used even with 2 kB RAM arduino (or 528 kB stm32). Are there any pitfalls in this claim?

2 long fields stored before string data instead of 1 might be significant obstacle for 2 kB RAM, so that's one example pitfall.

Has anyone seen sds use in embedded environment?

In many functions in file “sds.h”, the parameter “sds s” is dereferenced without checking if it is NULL. The same error is also present in some functions in file “sds.c”, such as: sdscat, sdsMakeRoomFor, sdsRemoveFreeSpace, sdsdup, sdsupdatelen, sdscatrepr, sdscmp, sdstoupper, sdstolower, sdsrange, sdstrim, sdscatfmt, sdsclear, sdslen, sdscatvprintf, sdscatprintf, sdscpy, sdscpylen, sdscatsds, sdscatlen, sdsgrowzero, sdsIncrLen, sdsAllocSize e sdsAllocPtr.

This functions should check for a parameter with value NULL and possibly return an error code in such case.

Minimal example:

int sdsTest(void) {
        sds x = NULL;
        test_cond("Create a string and obtain the length",
            sdslen(x) == 3 && memcmp(x,"foo\0",4) == 0)

    sdsfree(x);
    test_report();
    return 0;
}

Forcing the variable “sds s = NULL” while running the test programs generates a segmentation fault (due to the attempt to dereference NULL).

Hi @antirez,
according to the man page

realloc()  returns  a  pointer  to the newly allocated memory, which is
       suitably aligned for any kind of variable and  may  be  different  from
       ptr, or NULL if the request fails.  If size was equal to 0, either NULL
       or a pointer suitable to be passed to free() is returned.  If realloc()
       fails the original block is left untouched; it is not freed or moved.

in sds.c at line 717:

vector = realloc(vector, ((*argc)+1)*sizeof(char*));

if realloc() fails to allocate the requested memory, at line 718 it may get segfault.

Hello team , I am cross-compiling sds for i.MX6UL SoC . I could successfully able to build sds-test . But now I have requirement of ".so".
Can anyone kindly suggest me what modifications needs to be done in Makefile to generate .so file .

Error I am getting : arm-poky-linux-gnueabi/bin/ld: cannot find -lsds

Thanks

May be will be useful add make install target.

I see there is a fix for version 1 for c++ compiler. Is there anything similar for version 2?
Facing compilation issues, like below:

Am I missing something?

I'm trying to get another library (https://github.com/redis/hiredis) to compile with an app that is shooting for C89/C90 compliance. I'm running into problems. It uses sds.

The errors in sds that are preventing compilation are things like the following:

sds.h:48:10: warning: ISO C90 does not support flexible array members [-Wpedantic]
char buf[];

sds.h:86:8: error: unknown type name ‘inline’
static inline size_t sdslen(const sds s) {

sds.c:640:52: warning: ISO C90 does not support ‘long long’ [-Wlong-long]

Is there any chance that sds (being a low level library) can be changed to become C90 compliant?

Related Issue:
redis/hiredis#494

There is something about sdsMakeRoomFor that leaks memory, at least according to valgrind:

==31090== HEAP SUMMARY:
==31090==     in use at exit: 40 bytes in 2 blocks
==31090==   total heap usage: 33 allocs, 31 frees, 526 bytes allocated
==31090== 
==31090== 13 bytes in 1 blocks are definitely lost in loss record 1 of 2
==31090==    at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31090==    by 0x401B51: sdsMakeRoomFor (sds.c:160)
==31090==    by 0x40132A: main (sds.c:926)
==31090== 
==31090== 27 bytes in 1 blocks are definitely lost in loss record 2 of 2
==31090==    at 0x4C2CE8E: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31090==    by 0x401B51: sdsMakeRoomFor (sds.c:160)
==31090==    by 0x401CF0: sdscatlen (sds.c:269)
==31090==    by 0x401EFA: sdscatvprintf (sds.c:342)
==31090==    by 0x401FA6: sdscatprintf (sds.c:367)
==31090==    by 0x4026AE: sdscatrepr (sds.c:602)
==31090==    by 0x40126A: main (sds.c:915)
==31090== 
==31090== LEAK SUMMARY:
==31090==    definitely lost: 40 bytes in 2 blocks
==31090==    indirectly lost: 0 bytes in 0 blocks
==31090==      possibly lost: 0 bytes in 0 blocks
==31090==    still reachable: 0 bytes in 0 blocks
==31090==         suppressed: 0 bytes in 0 blocks

It might be worth considering only allocating strings of a capacity that is a power of two (including header) to reduce memory fragmentations:

A simple code snippet to round up to the next power of two:

unsigned int v; // compute the next highest power of 2 of 32-bit v

--v;
v |= v >> 1;
v |= v >> 2;
v |= v >> 4;
v |= v >> 8;
v |= v >> 16;
++v;

I can make the code contribution if you'd like.

By running a static analyzer, I got this information and I'd like to share.

[src/sds.c:159]: (error) Common realloc mistake: 'sh' nulled but not freed upon failure
[src/sds.c:717]: (error) Common realloc mistake: 'vector' nulled but not freed upon failure

Thanks
Carlos Tangerino

There exist platforms that use a C++ compiler even for C code. Regardless of that, I think it would be a good idea to make sds compile out of the box with C++ compilers...

Use it like this:

sds s = sdsfromlonglong(10000);
printf("%d\n", (int) sdslen(s));

output> 5

Why did you get the number 5? This simply can not be

sds s1 = sdsnew("abcd");
sds s2 = sdsempty();
s[1] = 1;
s[2] = 2;
s[3] = '\n';
s2 = sdscatrepr(s2,s1,sdslen(s1));
printf("%s\n", s2);

output> "a\x01\x02\n"

Why the array s is not created but variables are assigned there? And it is not used in this code.

You have these errors, correct

I did some investigation on Redis source code 4.0 while I was doing my job. Something about sdshdr5 and sdshdr8 storage had raised my curiosity and something really confused me popped up. Here are the steps to reproduce the scenario:

1. Open redis cli
2. Type SET key value
3. So here from my perspective and observation, THE value has been stored with sdshdr8 and THE key has been stored with sdshdr5(through dbAdd->sdsdup)
4. I was guessing with the help of MEMORY USAGE, THE key should be analysed as sdshdr5, while THE key was passed via c->argv[2]->ptr, it will be first encapsulated with method createdEmbeddeStringObject which adopt sdshdr8 as the struct type. So it will be analysed as sdshdr8 rather than sdshdr5.
   So, could anyone explain what is the actual reason for this and will this affect the accuracy of MEMORY USAGE command? Appreciated

In sdssplitlen(), slots starts with a default of 5. Then
tokens = malloc(sizeof(sds)*slots);
or generally 40 bytes.

However, if (len == 0) { *count = 0; return tokens;}

How do we free the 40 bytes? sdsfreesplitres() will use a count of 0.

Am I wrong that this is technically a memory leak? Of course, this is a situation that will probably never come up, since who splits a string with 0 chars, but still, it looks incorrect.

Why do we allocate memory to tokens before the test for len == 0?

Hi Salvatore,

I came across some unexpected behaviour while using the sdsMakeRoomFor() and sdsrange() functions. Please have a look below.
PS: It might be my one misuse of the library, but curious to hear what you think.

$ echo "test" >> test_file.txt

#include <stdio.h>
#include "sds/sds.h"

#define BUFSIZE 4096

static void expected_behaviour(FILE* const f) {
  sds s = sdsnew("abc");
  s     = sdsMakeRoomFor(s, BUFSIZE);

  while (fgets(s, BUFSIZE, f)) {
    sdsrange(s, 0, 2);
    printf("%s", s); // prints: tes
  }
  printf("\n");
  sdsfree(s);
}

static void unexpected_behaviour(FILE* const f) {
  // Can sdsempty() be used here?
  sds s = sdsMakeRoomFor(sdsempty(), BUFSIZE);

  while (fgets(s, BUFSIZE, f)) {
    // Why isn't sdsrange() working as expected?
    sdsrange(s, 0, 2);
    printf("%s", s); // prints: test\n
  }
  printf("\n");
  sdsfree(s);
}
int main(void) {
  FILE* const f = fopen("test_file.txt", "r");  // assume valid file
  expected_behaviour(f);
  fseek(f, 0, SEEK_SET);
  unexpected_behaviour(f);
  fclose(f)
  return 0;
}

As I've read in the reference and source when you pass an sds string to these two functions, you always have to assign the pointer back.

Is there any workaround to make these functions work in-place? That is: pass them an sds and have it modified? (without changing the object's pointer)

When compiling with UndefinedBehaviorSanitizer and running the tests (CC=clang-6.0 CFLAGS='-fsanitize=undefined', the following error is raised:

[...]
6 - sdscatprintf() seems working in the base case: PASSED
sds.c:446:23: runtime error: negation of -9223372036854775808 cannot be represented 
in type 'long long'; cast to an unsigned type to negate this value to itself
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior sds.c:446:23 in
[...]

Hi, why in sds.h you have (static inline functions)? Also why sdshdr* placed in header? I try to link sds.h into C++ project with -Wpedantic flag and receive errors like "warning: ISO C++ forbids zero-size array in sdshdr* structs".

In the example:

sds s1 = sdsnew("abcd");
sds s2 = sdsempty();
s[1] = 1;
s[2] = 2;
s[3] = '\n';
s2 = sdscatrepr(s2,s1,sdslen(s1));
printf("%s\n", s2);

output> "a\x01\x02\n"

We are editing indices of s but that is not declared, s should be s1.

The README states that sdstrim returns void, but in fact it returns sds.

In the function sdsnewlen we can see the test on a pointer that was already used as in:
sh = s_malloc(hdrlen+initlen+1);
if (!init)
memset(sh, 0, hdrlen+initlen+1);
if (sh == NULL) return NULL; <-----if this can be null, memset breaks

In my case I wanted to use %+i. Is this not planned to be added, or is this just not a high priority?

I've detected two typos at https://github.com/antirez/sds
Please search for:

• simple gaol of representing
• In many progrems it is useful

There's a possible memleak and NULL ptr usage in sds.c:40 in sdssplitargs:

vector = s_realloc(vector,((*argc)+1)*sizeof(char*));
vector[*argc] = current;

Realloc here might return NULL and thus NULLify vector while losing the reference to original object. I believe something like the following should be done:

char** new_vector = s_realloc(vector,((*argc)+1)*sizeof(char*));
if (new_vector == NULL) goto err;
vector = new_vector;
vector[*argc] = current;

The sdsnewlen and sdsMakeRoomFor function implemented in sds.c is quite similiar to those in the redis. Thus, it's very likely that this integer overflow in CVE-2021-21309 also affects sds.
Would you can help to check if this bug is true? If it's true, I'd like to open a PR for that if necessary. Thank you for your effort and patience!
And here is the patch for CVE-2021-21309 for your reference if this issue needs to be fixed.