I just started learning C. I completely have no idea where I miss. It keeps saying Invalid write of size 1 error. How do I fix this ? Thank you very much.

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

char *mystrcat(char *dest, const char *src)
{

    char *origdest = realloc(dest,(strlen(dest)   strlen(src))*sizeof(char)   1);
    while (*dest)
    {
        dest  ;
    }

    while (*src)
    {
        *dest   = *src  ; // Copy the source
    }
    *dest = 0;

    return origdest;
}


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

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

    return 0;
}

CodePudding user response：

There are a few errors/mistakes I would suggest you the following:

1. Typecase the malloc call with a specific pointer (malloc returns void* which is a kind of generic one, so you have to typecast it to char* in this case)
2. Same goes for realloc.
3. It's better to use malloc to allocate memory for new pointer.
4. Put a NULL check filter after alloc calls to check whether memory is allocated to your pointer or not.

CodePudding user response：

char *origdest = realloc(dest,(strlen(dest)   strlen(src))*sizeof(char)   1);

After this point, one of the three things happen:

1. Resize-in-place has happened. dest == origdest.
2. Resize-in-place did not happen, copy-to-new-buffer happened instead. origdest now points to a suitably sized block of memory that contains a copy of what dest used to point at, and dest is an invalid pointer. Any operation on dest is undefined behaviour.
3. Resize has failed. origdest == NULL.

Your program will only work if case 1 takes place.

If case 3 takes place, you should indicate an error and exit, or recover. The latter is hardly an option for a simple exercise, so insert

assert (origdest != NULL);

below the call to realloc.

Now if case 2 takes place, you want to restore the invariant which is guaranteed by case 1 and which your code relies on.

dest = origdest;

should do the job.

I would recommend to rename origdest to newdest, because this is a new destination, not the original one.

Finally, sizeof(char) is 1 by definition, just omit it, or at least use it consistently if you do not want to rely on this fact. (strlen(dest) strlen(src) 1) * sizeof(char) reflects the intent better.

Live demo

CodePudding user response：

Valgrind doesn't like your code as it just prints Aatami, which is not that correct.

Your implementation of strcat() is very inefficient and buggy.

strcat() never realloc() the data, it assumes that dest must have enough space available. So, you should not call realloc() for dest. Allocate enough space for dest in the main() function.

Here's my implementation of fast_strcat(), it's time complexity is O(n), where n is the length of src.

#include <stdio.h>
#include <stdlib.h>

void fast_strcat(char *dest, const char *src, size_t *size)
{
    if (dest && src && size)
        while ((dest[*size] = *src  ))
            *size  = 1;
}

int main(void)
{
    char *str = calloc(14, sizeof(char));
    size_t len = 0;
    fast_strcat(str, "Aatami", &len);
    fast_strcat(str, "Beetami", &len);

    printf("%s\n", str);
    printf("Length of `str` = %zu\n", len);
    
    free(str);
    return EXIT_SUCCESS;
}

Output:

AatamiBeetami
Length of `str` = 13

Valgrind:

==19656== HEAP SUMMARY:
==19656==     in use at exit: 0 bytes in 0 blocks
==19656==   total heap usage: 2 allocs, 2 frees, 1,040 bytes allocated
==19656== 
==19656== All heap blocks were freed -- no leaks are possible
==19656== 
==19656== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)