errors concatenating bytes from a block of bytes using memcpy

Question

On occasion, the following code works, which probably means good concept, but poor execution. Since this crashes depending on where the bits fell, this means I am butchering a step along the way. I am interested in finding an elegant way to fill bufferdata with <=4096 bytes from buffer, but admittedly, this is not it.

EDIT: the error I receive is illegal access on bufferdata

unsigned char        buffer[4096] = {0};
char *bufferdata;

bufferdata = (char*)malloc(4096 * sizeof(*bufferdata));
if (! bufferdata)
    return false;

while( ... )
{

    // int nextBlock( voidp _buffer, unsigned _length );
    read=nextBlock( buffer, 4096);

    if( read > 0 )
    {
        memcpy(bufferdata+bufferdatawrite,buffer,read);

        if(read == 4096) {

            // let's go for another chunk
            bufferdata = (char*)realloc(bufferdata, ( bufferdatawrite + ( 4096 * sizeof(*bufferdata)) ) );
            if (! bufferdata) {
                printf("failed to realloc\n");
                return false;
            }

        }

    }
    else if( read<0 )
    {
        printf("error.\n");
        break;
    }
    else {
        printf("done.\n");
        break;
    }
}


free(bufferdata);

Answer 1

A few comments:

Please define or const 4096. You will get burned if you ever need to change this. realloc chaining is an extremely inefficient way to get a buffer. Any way you could prefetch the size and grab it all at once? perhaps not, but I always cringe when i see realloc(). I'd also like to know what kZipBufferSize is and if it's in bytes like the rest of your counts. Also, what exactly is bufferdatawrite? I'm assuming it's source data, but I'd like to see it's declaration to make sure it's not a memory alignment issue - which is kinda what this feels like. Or a buffer overrun due to bad sizing.

Finally, are you sure they nextBlock isn't overruning memory some how? This is another point of potential weakness in your code.

Answer 2

It's hard to tell where the error is, there's some code missing here and there.

if(read == 4096) { looks like a culprit, what if nextBlock, returned 4000 on one iteration, and 97 on the next ? Now you need to store 4097 bytes but you don't reallocate the buffer to accomodate for it.

You need to accumulate the bytes, and realloc whenever you pass a 4096 boundary. something like:

#define CHUNK_SIZE 4096
int total_read = 0;
int buffer_size = CHUNK_SIZE ;
char *bufferdata = malloc(CHUNK_SIZE );
char buffer[CHUNK_SIZE];
while( ... )
{

    // int nextBlock( voidp _buffer, unsigned _length );
    read=nextBlock( buffer, CHUNK_SIZE );

    if( read > 0 )
    {
        total_read += read;
        if(buffer_size < total_read) {
           // let's go for another chunk
            char *tmp_buf;
            tmp_buf= (char*)realloc(bufferdata, buffer_size + CHUNK_SIZE );
            if (! tmp_buf) {
                free(bufferdata);
                printf("failed to realloc\n");
                return false;
            }
            buffer_data = tmp_buf;
            buffer_size += CHUNK_SIZE ;

        }
        memcpy(bufferdata+total_read-read,buffer,read);
      }
      ... 
    }