# C-Style unsigned char parsing and manipulation

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 );
}

• This question is a bit offtopic since the code doesn't work, and that's forbidden (cf. the faq). But since there's also an interesting question about code correctness, it could be kept here. Mar 5, 2012 at 14:43
• What is the definition of viByteBuf ? Mar 5, 2012 at 16:36
• viByteBuf is just a struct with two properities, str (unsigned char) and len (unsigned int). It's supposed to represent a buffer which can dynamically grow/shrink. Mar 5, 2012 at 18:08

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.

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.

• Hmm...why do you think I asked for a review on this site? I appreciate your objective analysis, but that last snippet there was completely unnecessary. Also, the reason why I'm not using the std::vector is because I'm writing this on Android, AND I want to actually learn a thing or two rather than just mindlessly pass values to a vector. Mar 7, 2012 at 0:33
• @Holland: Code review site is for working code. This is obviously very broken. Mar 7, 2012 at 0:38
• Also, you don't even offer a solution to the problems presented, which means that your answer, while tearing apart my code and showing me what I've done wrong, hasn't taught me a damn thing, because you don't offer a solution. Mar 7, 2012 at 0:40
• @Holland: Easy solution use std::string rather than trying to write your own. Mar 7, 2012 at 0:47
• Should probably be moved to stackoverflow.com Mar 8, 2012 at 17:40