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

• Quentin Pradet: 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.
• Joey Adams: What is the definition of viByteBuf ?
• Holland: 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.

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.

• Holland: 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.
• Loki Astari: @Holland: Code review site is for working code. This is obviously very broken.
• Holland: 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.
• Loki Astari: @Holland: Easy solution use std::string rather than trying to write your own.
• karlphillip: Should probably be moved to stackoverflow.com