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

I get the above debugging problem from the function readString. I b开发者_StackOverflowelieve 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;

    }

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;

}

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.