1
\$\begingroup\$

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 );
}
\$\endgroup\$
  • 1
    \$\begingroup\$ 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. \$\endgroup\$ – Quentin Pradet Mar 5 '12 at 14:43
  • \$\begingroup\$ What is the definition of viByteBuf ? \$\endgroup\$ – Joey Adams Mar 5 '12 at 16:36
  • \$\begingroup\$ 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. \$\endgroup\$ – Holland Mar 5 '12 at 18:08
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Holland Mar 7 '12 at 0:33
  • 2
    \$\begingroup\$ @Holland: Code review site is for working code. This is obviously very broken. \$\endgroup\$ – Martin York Mar 7 '12 at 0:38
  • \$\begingroup\$ 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. \$\endgroup\$ – Holland Mar 7 '12 at 0:40
  • 1
    \$\begingroup\$ @Holland: Easy solution use std::string rather than trying to write your own. \$\endgroup\$ – Martin York Mar 7 '12 at 0:47
  • 1
    \$\begingroup\$ Should probably be moved to stackoverflow.com \$\endgroup\$ – karlphillip Mar 8 '12 at 17:40

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.