search

antirez / sds

Simple Dynamic Strings library for C
BSD 2-Clause "Simplified" License
4.89k stars 473 forks source link

Fixed sdscat*() bug: read memory error #118

Open iiol opened 4 years ago

iiol commented 4 years ago

Example:

sds str;

str = sdsnew("Hi ");
str = sdscat(str, str);
printf("%s\n", str); // not "Hi Hi "
UmanShahzad commented 4 years ago

You can simplify this with memmove.

cassepipe commented 3 years ago

Wouldn't a call to memmove affect performance for big strings ? Is it not better to mention in the doc that sdscat should not take two same pointers ?

UmanShahzad commented 3 years ago

It depends; some implementations of memmove will check for an overlap first, and if there isn't one will delegate to memcpy.

Example of musl libc doing this: https://git.musl-libc.org/cgit/musl/tree/src/string/memmove.c#n15

fractalb commented 3 years ago

This is a non-issue. This is not the intended way of using sdscat function. It's defined with this prototype: sds sdscatlen(sds s, const void *t, size_t len) which promises not to modify the second argument t. It forbids the case where s == t. Maybe, add a comment about this limitation

UmanShahzad commented 3 years ago

Then how is the overlap case ever going to be supported?

Either brand new interfaces with the exact same code but with memcpy -> memmove (and there are many functions calling into this), thus significantly complicating the interface, or remove const and break backwards compatibility at the interface (warnings will be generated for users who were passing in const arguments into t).

The simplest solution is to just use memmove, which has practically no extra cost because it will still delegate to memcpy, and note in the comments this edge case.