Simple Dynamic Strings library for C

They almost never have examples(Is that some sort of rule?), they have no "quick summary of the shit you use 95% of the time", and they get generally written as a novel, seemingly from the perspective of the developer of the util rather than consumer.

Mind you, I have used man pages many times before, but only because it was the best source of information, not because it was a particularly efficent one.

The Markdown README of this string library is a thousand times better than any man page I have seen.

I often leave this out of my own code, but not without feeling somewhat guilty about it.

Consider what happens if an attacker controls initlen passed to sdsnewlen() (also via sdsnew() if he controls the initial string). He can overflow the arithmetic on malloc/calloc. Now either the memcpy or the NUL-byte assignment can write out of bounds, which is potentially exploitable, especially if the attacker has a nearly unbounded number of attempts (which they often do as far as online-facing services go) or if they can do things to affect the program's memory layout -- which they likely can if they're feeding the program with data.

But if these two aren't enough for a successful exploit (I wouldn't make any bets!), the initlen is assigned to sh->len (whoops, conversion to signed... and possible overflow), which then can completely throw off the arithmetic in sdsMakeRoomFor. Here I would actually be quite willing to bet an attacker can arrange these numbers so that he can do a more controlled out-of-bounds write in one of the functions that rely on sdsMakeRoomFor.

These two functions aren't necessarily the only problematic ones; they're just the ones I noticed after a minute of scanning.

And I don't really agree with what eliteraspberrie said; using the right types is a good start but it's absolutely not fine until after you actually do all the overflow checking; for an example of this, see the OpenBSD man page for malloc: http://www.openbsd.org/cgi-bin/man.cgi?query=malloc

Rule of thumb: whenever you do any arithmetic, ask yourself, can it overflow (how do you know it doesn't?) and who's in charge of making sure it doesn't? In this case it's your API's responsibility. This is exactly the same thing you do whenever you call a function: you need to know if it can fail, and if it can, you need to check the return value and do the right thing.

In the years I've spent reading CVEs (and the broken code that caused the alert), proof-of-concept exploits as well as real world exploits, I've learned that some incredibly subtle and seemingly insignificant things can be exploited. As a general rule, don't worry about exploitability, it's usually not worth your time to prove one way or another. Just fix the code whenever you see something that could go wrong. Make it easy for the next guy who audits your code; so he too can tell that your code is secure, just by seeing the right checks in the right place.

If you need a refresher on security issues in C code, I recommend Chapter 6 from The Art of Software Security Assment, made available free of charge by the authors: http://ptgmedia.pearsoncmg.com/images/0321444426/samplechapt...

It's a good read for anyone doing C these days, and covers a good deal of problems, including the one discussed here. It comes with snippets of known vulnerable real world code too.

    #include <stddef.h>

    int buf_offset = (int)offsetof(struct sdshdr, buf);
    struct sdshdr *sh = s - buf_offset;

    struct header {
        int i;
        char c;
        char data[];
    };

    #include <stdio.h>
    #include <stddef.h>

    struct header {
        int i;
        char c;
        char data[];
    };

    int main(void)
    {
        struct header h = {0};
        printf("offsetof %zu\n", offsetof(struct header, data));
        printf("sizeof %zu\n", sizeof h);
        printf("start %p, data start %p, delta %d\n",
                (void *)&h, (void *)(h.data), (int)((char *)h.data - (char *)&h));

        return 0;
    }

    cleanup(state);
    error(state);

Just pull them each out into a function. You're still DRY and don't lose flow control.

In my honest opinion, there is never a need for goto.

http://www.u.arizona.edu/~rubinson/copyright_violations/Go_T...

According to Dijkstra, the really evil gotos are the ones that force you to keep a track of the whole execution path in order to figure out the current state of the program. Its much easier when you can look at a line of code and statically know what the program state will be like at that point.

A for loop is better than goto because you can look at a line inside it and instantly know that it will run a number of times, with the index changing in increasing order.

gotos for resource cleanup are good because you can look at a given line of code and know what resources still need to be freed.

Code that gets rid of gotos and break statements by blindly replacing them with tons of flags is just as bad as gotos because you still need to look at the whole execution path to figure out the state in the flags.

windows (for example) has had a comprehensive b-string library (type is called BSTR) for about 20 years - due to it's age and provenance it has the downside of thinking a character is a 16-bit value...

Not nice I know.

Consistency is a tool that is often useful to promote program correctness. You're suggesting using it to accomplish the reverse.

If it's important aesthetically that all sds operations be consistent in this regard, you can structure the allocation interface in the same way:

  sdserror sdsnew(sds*);

  sds mystring = NULL;
  sdsnew(&mystring, "Hello World!");

> And you still want to access the old value if reallocation failed.

If you want to provide commit-or-rollback semantics, you could signal error via return value rather than by replacing s with NULL:

  if (sdscat(&s,"Some more data") != SDS_SUCCESS) {
    /* failure path; s is unchanged */
  } else {
    /* s now has "Some more data in it" */
  }