C programming problem in dynamic memory allocation

Question

The problem should be simple, but I have spent hours on this and cannot see what is wrong in my logic. The output works as it should, but Valgrind prints memory issues that should be fixed. I have added the origdest = (char*)realloc(origdest, strlen(origdest) + i * sizeof(char)); code to the while loop, my question is why doesn't this dynamically adjust the memory? The exact error given by Valgrind is

==9== Invalid write of size 1
==9==    at 0x1087E2: mystrcat (mystrcat.c:18)
==9==    by 0x10883C: main (mystrcat.c:34)
==9==  Address 0x522d046 is 6 bytes inside a block of size 7 free'd
==9==    at 0x4C31D2F: realloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9==    by 0x1087C2: mystrcat (mystrcat.c:17)
==9==    by 0x10883C: main (mystrcat.c:34)
==9==  Block was alloc'd at
==9==    at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==9==    by 0x108811: main (mystrcat.c:31)

char *mystrcat(char *dest, const char *src)
{
    char *origdest = dest;
    
    while(*dest) {
        dest++;
    }
    
    int i = 1;

    while (*src) {
        origdest = (char*)realloc(origdest, strlen(origdest) + i * sizeof(char));
        *dest++ = *src++;  // Copies character and increases/moves pointer
        i++;
    }
    
    *dest = 0;

    return origdest;
}

int main(void)
{
    char *str = malloc(7);
    strcpy(str, "Mydogs");

    str = mystrcat(str, "arecool");
    printf("%s\n", str);
    free(str);
}

After reallocating, you have to assume that the allocated space moved. The `dest` pointer is no longer valid. You must reestablish where the extra material should be copied to. — Jonathan Leffler, Feb 16 '21 at 13:35

ryyker · Accepted Answer · 2021-02-16T22:42:10.120

This statement:

Address 0x522d046 is 6 bytes inside a block of size 7 free'd is saying that the realloc() called following these statements results in the old pointer pointing to freed memory.

after this segment:

char *origdest = dest;
    
while(*dest) {
        dest++;
}

EDIT to address comment "what is specifically wrong with the code and what could be changed to make it work?"

The explanation of my first observation above is that once the pointer to allocated memory is moved, as you have done, the memory allocation tables no longer have an accurate location of that memory, making that memory un-freeable.

Your stated goal here is to create a version of strcat(), so using realloc() is a reasonable approach, but to use it safely allocate the new memory into a temporary buffer first, then if allocation fails, the original memory location still exists, and can be freed.
One other small change that makes a big difference is how i is initialized. If 1 is used, it places the beginning of the second string one extra position further in memory, leaving a \0 character just after the first string, effectively making it the end of the resultant string. i.e. you would never see the appended part of the string: In memory it would look like this:

|M|y|d|o|g|s|\0|a|r|e|c|o|o|l|

Then over-flow your buffer when attempting to place another NULL terminator at the and of the concatenated buffer, resulting in undefined behavior.

The following adaptation of your code illustrates these, along with some other simplifications:

char *mystrcat(char *dest, const char *src)
{
    char *temp = NULL;
    
    int i = 0;//changed from i = 1 as first location to 
              //copy to is len1, not len1 + 1
              //Note, starting at len1 + 1 would leave a NULL character
              //after "Mydogs", effectively ending the string
    //the following are simplifications for use in realloc()
    int len1 = strlen(dest);
    int len2 = strlen(src);

    //call once rather than in a loop.  It is more efficient.
    temp = realloc(dest, len1+len2+1);//do not cast return of realloc
    if(!temp)   
    {
        //handle error 
        return NULL;
    }
    dest = temp;
    while(*src)
    {
        dest[len1 + i] = *src;
        i++;
        src++;
    }
    dest[len1 + i] = 0;//add null terminator

    return dest;     
}

    int main(void)
    {
        char *temp = NULL;          
        char *str = malloc(7);
        if(str)//always a good idea to test pointer before using
        {
            strcpy(str, "Mydogs");
            temp = mystrcat(str, "arecool");
            if(!temp)
            {
                free(str);
                printf("memory allocation error, leaving early");
                return 0;
            }
            str = temp;
            printf("%s\n", str);
            free(str);
        }
        return 0;
    }

Why it is not correct to cast the return of c-m-realloc() in C.

Your first sentence doesn't make sense. After mutating `dest`, `origdest` *does* still point at the original allocation. The problem is that after the `realloc`, `dest` now points to memory that has been freed, as Valgrind also reports. — trent, Feb 16 '21 at 14:51
Sir, thank you for your existence. The main function was given and shouldn't be modified from what it is, but with your help I was able to make everything work. — John, Feb 16 '21 at 15:05
@John - If this addresses your problem you can mark it as accepted by clicking the hollow check. — ryyker, Feb 16 '21 at 15:06
@John - By the way, If you are being told to leave the main function as is, ask your instructor how then to handle an error coming from a failed call to `malloc()` without testing the result. — ryyker, Feb 16 '21 at 15:11
@trentcl - Thank you for pointing out the wrong wording. Its been edited.. — ryyker, Feb 16 '21 at 15:14
I still don't think this is an accurate description of what's wrong with the original code. It's not the *new* pointer that now points to freed memory, it's the *old* one. There is no "orphaned" memory here (except when `realloc` fails, which there is no evidence to suggest). Un-freeable memory results in leaks, not memory errors. — trent, Feb 16 '21 at 15:41

score 1 · Answer 2 · answered Feb 16 '21 at 13:41

Here you move to the end of the original string:

    while(*dest)
        dest++;

Here you allocate some new memory, but dest still points to the end of the original string. So you are overwriting memory after the end of the original string. Since you are reallocating, the original string may not even exist anymore at the previous location you are writing to, because realloc can move the data to a completely new location.

    while (*src)
    {
        origdest = (char*)realloc(origdest, strlen(origdest) + i * sizeof(char));
        *dest++ = *src++;  // Copies character and increases/moves pointer
        i++;
    }

I still don't understand... I do not remember when I was this frustrated with a coding problem lol. Could someone give any tip on what is specifically wrong with the code and what could be changed to make it work? Thank you... — John, Feb 16 '21 at 13:57

C programming problem in dynamic memory allocation

2 Answers2