C-Style unsigned char parsing and manipulation

Question

Note that I'm using a C++ compiler ( hence, the cast on the calloc function calls) to do this, but the code is essentially C.

Basically, I have a typedef to an unsigned char known as viByte, which I'm using to create a string buffer to parse a file from binary (a TGA file, to be exact - but, that's irrelevant).

I'm writing basic functions for it right now; append, prepend, new, etc.

The problem is that, on the first iteration of the first loop in viByteBuf_Prepend, I get a segmentation fault. I need to know why, exactly, as this is something which could keep me up all night without some pointers (pun intended).

I also would like to know if my algorithms are correct in terms of how the buffer is pre-pending the viByte string. For example, I have a feeling that using memset too much might be a bad idea, and whether or not my printf format for the unsigned char is correct (I have a feeling it isn't, as nothing is getting output to my console).

Compiling on GCC, Linux.

#ifdef VI_BYTEBUF_DEBUG
void viByteBuf_TestPrepend( void )
{
    viByteBuf* buf = viByteBuf_New( 4 );

    buf->str = ( viByte* ) 0x1;

    printf(" Before viByteBuf_Prepend => %uc ", buf->str);

    viByteBuf_Prepend( buf, 3, ( viByte* ) 0x2 );

    printf(" After viByteBuf_Prepend => %uc ", buf->str);
}
#endif

viByteBuf* viByteBuf_New( unsigned int len )
{
    viByteBuf* buf = ( viByteBuf* ) calloc( sizeof( viByteBuf ), 1 );

    const int buflen = len + 1;

    buf->str = ( viByte* ) calloc( sizeof( viByte ), buflen );
    buf->len = buflen;

    buf->str[ buflen ] = '\0';

    return buf;
}

void viByteBuf_Prepend( viByteBuf* buf, unsigned int len, viByte* str )
{
    unsigned int pos, i;
    const unsigned int totallen = buf->len + len;
    viByteBuf* tmp = viByteBuf_New( totallen );
    viByte* strpos = buf->str;


    memset( tmp->str, 0, tmp->len );

    int index;

    for( i = 0; i < buf->len; ++i )
    {

       index = ( buf->len - i ) - 1;

       *strpos = buf->str[ 0 ];
       ++strpos;
    }

    memset( buf->str, 0, buf->len );

    printf( "%uc\n", buf->str );

    i = totallen;

    for ( pos = 0; pos < len; ++pos )
    {
        tmp->str[ pos ] = str[ pos ];
        tmp->str[ i ]   = buf->str[ i ];

        --i;
    }

    memset( buf->str, 0, buf->len );

    buf->len = tmp->len;

    memcpy( buf->str, tmp->str, tmp->len );

    viByteBuf_Free( tmp );

    //memset(  )
    //realloc( ( viByteBuf* ) buf, sizeof( viByteBuf ) * tmp->len );
}

Answer 1

Your problem is here:

buf->str[ buflen ] = '\0';

You allocate a buffer (str) of buflen bytes.
This means you can index it (str) from 0 -> (buflen-1).

Thus the accesses above is writing one past the end of the buffer and corrupting memory.
You probably meant:

buf->str[ len ] = '\0';

Since you are using C++ compiler. You should use std::vector<viByte> or std::basic_string<viByte>.

This kind of bug is easy to introduce and unless you have good unit test hard to find. So why not use a class that already is unit test and has had 10 years of millions of people lookign at it to make sure it works correctly.

Other comments:

Broken and illegal.

    buf->str = ( viByte* ) 0x1;

Wrong. buf->str is a pointer and it was expecting a unsigned integer (they are not the same). Use %p to print a pointer value.

    printf(" Before viByteBuf_Prepend => %uc ", buf->str);

Broken and illegal (0x2 is not yours)

    viByteBuf_Prepend( buf, 3, ( viByte* ) 0x2 );

No check for NULL. (calloc can return NULL when it fails).

    viByteBuf* buf = ( viByteBuf* ) calloc( sizeof( viByteBuf ), 1 );

Again no check for NULL.

    buf->str = ( viByte* ) calloc( sizeof( viByte ), buflen );

Is this really what you want? Buflen includes the portion with the '\0' character. You need to be consistent and decide if len does or does not include the null.

    buf->len = buflen;

As pointed out above (write past the end of the buffer).

    buf->str[ buflen ] = '\0';

How do you handle failure?
As pointed out above calloc can fail. How do you propagate that failure to the caller.

    viByteBuf* tmp = viByteBuf_New( totallen );

Why are you nulling the whole array. You are just about to write over it.
Seems like a complete waste of time.

    memset( tmp->str, 0, tmp->len );

Looks like this loop is trying to reverse the string in place.
Which will fail as by the time you overwrite the first half the second half is a copy of the back half. Luckily it completely fails because you don't actually use the index variable.

    for( i = 0; i < buf->len; ++i )
    {

       index = ( buf->len - i ) - 1;

       *strpos = buf->str[ 0 ];
       ++strpos;
    }

Now that you have failed to reverse it. You then overwrite it again with 0 (presumably to cover up the failure to reverse it).

    memset( buf->str, 0, buf->len );

Illegal parameter to printf() again.

    printf( "%uc\n", buf->str );

More copying for unknow reason.

    memcpy( buf->str, tmp->str, tmp->len );

This code is so full of bugs. It has no write to work.