Run-Time Check Failure #2 - Stack around the variable 'start' was corrupted

Question

I get the above debugging problem from the function readString. I believe it has something to do with the way 'start' is defined in the function. The 0x07 in the array changes depending on the length of the following string. This string should say 'testing' in unicode.

int main(){
    char readbuffer[] = {0x07, 0x74, 0x65, 0x73, 0x74, 0x69, 0x6e, 0x67};
    char *buf = readbuffer;
    uint32_t *stringread = (uint32_t *) malloc(sizeof(uint32_t));
    *stringread = readString(buf);
    }

    uint32_t readString(char *buf)
    {
         uint32_t *start = (uint32_t *) malloc(sizeof(uint32_t));
         int len;

         len = protobuf_readVarint(buf, &buf);
         memcpy (&start, buf, len);
         buf += len;

         return start;

    }

score 1 · Answer 1 · answered Aug 15 '11 at 15:51

Peter answered your question. Besides I would advice you to use

len = max(protobuf_readVarint(buf, &buf),sizeof(uint32_t));

or catch when the first argument is bigger than the second because otherwise you write too much into the argument start. Also you have a memory leak which could be fixed with:

uint32_t readString(char *buf)
{
     uint32_t start;
     int len;

     len = max(protobuf_readVarint(buf, &buf),sizeof(uint32_t));
     memcpy ((void*)&start, buf, len);
     buf += len;

     return start;

}

Péter Török · Accepted Answer · 2011-08-15T16:01:25.450

Here

     memcpy (&start, buf, len);

you are attempting to copy the contents of buf to &start (the address of the pointer variable start, that is in fact an address on the stack) instead of start (referring to the address of the memory buffer start is pointing to). This is what corrupts your stack.

Apart from this, there are several other smaller issues in your code:

you have no bounds checking, so if len is > 4, memcpy will again silently write past the end of the memory block pointed to by start, corrupting memory;
you allocate a memory block within readString() which you never free, leading to a memory leak; if you are absolutely sure that len will never be more than 4, it would be simpler to use just a plain local variable:
```
 uint32_t start;
 ...
 memcpy (&start, buf, len);
```
note that in this case it is correct to pass &start to memcpy!
buf += len will have no effect outside readString() since buf is passed by value, thus changes to it within the function affect only its local copy, not the original.

len will be greater than 4. does this mean I have to use memcpy(start,..) And how should I allocate the memory for start properly? — franklin, Aug 15 '11 at 16:16
@franklin, you should first get the needed length from the call `len = protobuf_readVarint(...)`, then allocate a buffer of that length: `uint32_t *start = malloc(len)`. (assuming that `protobuf_readVarint()` returns the number of bytes read, not words or `uint32`s or something else.) — Péter Török, Aug 15 '11 at 16:59

Run-Time Check Failure #2 - Stack around the variable 'start' was corrupted

2 Answers2