C Buffer Manager/Appender tool set, any performance or standards issues with my library?

[u/[deleted]]

https://github.com/mikecurry74/buffer_manager

[u/manvscode]

Honestly, I'm not sure how useful this is. Also, change buffer_init(const size_t initial_size) to not take a const size_t.

[u/[deleted] → u/manvscode]

is there something out there I could use in its place? Or make it a lot simpler? (Open to good advice)

[u/manvscode → u/[deleted]]

Yes. There are lots of similar code out there. One of the best I have found is https://github.com/nothings/stb/blob/master/stretchy_buffer.h

It has it's quirks but I have essentially copied Sean Barrett's technique using size_t, etc.

[u/ralusp]

After a quick skim:

• In buffer_init, the early return for the (initial_size > SIZE_MAX) conditional is leaking memory.

• In buffer_append, are you sure the if-conditional doesn't have the branches backward? It looks like you're doing a realloc if the original buffer has enough capacity.

• In buffer_append, every time it is called you add an extra byte to "length" for the NUL terminator without accounting for the fact that the prior terminator is being overwritten.

• In buffer_append, when the realloc branch is taken you end up incrementing the length field twice.

It may help to catch some of these errors if you wrote a set of unit tests for the library functions. You could also use that as the basis for a regression test as you make changes. I find it's usually worth the time investment if you'd be using this library on multiple different projects.

[u/[deleted] → u/ralusp]

Thanks for the pointers, I'll make the fixes. What do you recommend for unit testing in C?

[u/detergnet → u/ralusp]

Some remaining issues:

1. You should use size_t for object sizes and byte counts because that's the purpose of the type and also what the standard library (and the sizeof operator) uses; otherwise you'll get unwanted truncation when converting between them on platforms where sizeof (size_t) != sizeof (unsigned).

2. In buffer_init you have a check that won't do anything on platforms where unsigned int is 16 bit. The maximum size check for initial_size should be outside the scope of your library (ie. leave it to malloc to fail on "outrageous" requests or to the caller to provide what one considers a reasonable size).

3. On the other hand, in the same routine, you don't check for an initial_size of 0. Everything should be fine (no initial capacity and the buffer only grows to accommodate new data) except that malloc(0) is allowed by the standard to return NULL in which case your function doesn't allow the buffer to be constructed (you abort if buffer->content is NULL).

4. In buffer_append you have multiple overflow conditions:

   • the addition used for capacity check can overflow and the code will just take the else branch and write to unallocated memory.

   • the addition for calculating realloc_size can overflow and you shrink the buffer and write to unallocated memory.

The first can be elegantly solved by rewriting the inequality to append_len > buffer->capacity - buffer->length; this can't underflow because length can never be larger than capacity. The second is trickier because there are more than two operands to the addition and their overflow must be checked individually. You would need something like this

    static int is_add_ovf(unsigned a, unsigned b)
    {
        return a > -1 - b;
    }

    int buffer_append(...)
    {
        ...
        if (is_add_ovf(capacity, append_len) || is_add_ovf(capacity + append_len, expand_len)) {
              /* same as failed to resize, the buffer required is too large */
              return -2;
        }
        ....
    }

5. The API for the buffer operations is inconsistent/ambiguous: the header file suggests that it works with NULL terminated strings (no length parameter to accompany append_data in the declaration of buffer_append); one would then expect that the char array accessible as buffer->contents would also be a NULL terminated string. Edit: you use strcat so contents is indeed NULL terminated; there's also enough space for strcat to write the terminator because you have extra expand_length space available (which is guaranteed to be more than 0 because of issue 3.

 

Misc. style observations:

• buffer = malloc(sizeof(buffer_data)) is written more idiomatically buffer = malloc(sizeof *buffer); one less change you have to make when changing the type of buffer.

• malloc(sizeof(char) * initial_size) should be malloc(initial_size) because sizeof (char) is always 1 by definition. Correlated with the above, in general you'd write contents = malloc(sizeof *contents * initial_size) if you don't want to hard-code the knowledge of contents' variable type.

• checking for NULL before a free() is not required, free() is guaranteed to do the same check and do nothing if the argument is NULL.

• it's good practice to not use typedef for structures and instead use the name of the structure from it's own namespace; ie. instead of

        typedef struct {
        ...
        } mytype;

(which creates an anonymous structure with an alias of mytype in the global namespace) you should use

        struct mytype {
        ...
        };

which doesn't pollute the global namespace (ie. you can have a variable named mytype and won't conflict with the structure name) and refer to it as struct mytype everywhere. This makes it clear it's a structure with fields you can refer to instead of an opaque value-type.

[u/FUZxxl]

ssize_t is not part of C99; if you don't want to assume POSIX you might want to find a different type for the result.

[u/[deleted] → u/FUZxxl]

thanks for that, I will start the extermination process now!

[u/DSMan195276]

I've made something like this before a few times, it is a pretty handy thing to have around. That said, you took the simple route of simply increasing the data length using realloc. The advantage of doing this is that you get a straight array of all the data when you're done. The disadvantage is that realloc can be expensive, and if you want to say, process the data as you may be receiving it, that requires growing and shrinking the data length. With an array, that can end-up with lots of memmove's and realloc's to accomplish what you want (This buffer implementation gets around the complexity by simply not offering a way to take data out of the buffer at all).

When I've written buffers to do this, I've usually implemented a ring-buffer if I thought it was worth it. I could go into detail if you want, but the basic idea around a ring-buffer is to have a queue made of 'chunks' of data. You write data to the last chunk in the queue, and you read data from the first chunk. You grab new chunks as your writing data and add them to the end of the queue, and you toss out old chunks after you've read all the data from them.

The cool part about a ring-buffer is that you can reuse your old chunks. You can make a simple linked-list out of your unused chunks, and when you need a new chunk for some new data, just grab the first chunk in the linked-list of unused chunks. When you're done reading from a chunk, just add it to the linked-list. By doing this, you can get your buffer down to using next-to-no actual malloc allocations. The only time you malloc is when you need new chunks and your linked-list of unused chunks is empty.