My own strcat function with pointers does not work right

Question

I am new to pointers and want to learn them well. So this is my own attempt to write my strcat function. If I return just a it prints some binary things (I think it should print the solution), If I return *a it says seg fault core dumped I couldn't find the error. Any help is accepted thanks.

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

int main() {
    char *strcaT();

    char *a = "first";

    char *b = "second";

    printf("%s", strcaT(a, b));

    return 0;
}

char *strcaT(char *t, char *s) {   
    char buffer[strlen(t) + strlen(s) - 1];

    char *a = &buffer[0];

    for (int i = 0; i < strlen(s) + strlen(t); i++, t++) {
        if (*t == '\n') {
            for (int i = 0; i < strlen(s);i++) {
                buffer[strlen(t) + i] = *(s + i);
            }
        }
        buffer[i] = *(t + i);
    }
    return a;
}

Answer 1

The code has multiple cases of undefined behavior:

• you return the address of a local array in strcaT with automatic storage, which means this array can no longer be used once it goes out of scope, ie: when you return from the function.

• the buffer size is too small, it should be the sum of the lengths plus 1 for the null terminator. You write beyond the end of this local array, potentially overwriting some important information such as the caller's framce pointer or the return address. This undefined behavior has a high chance of causing a segmentation fault.

• you copy strlen(t)+strlen(s) bytes from the first string, accessing beyond the end of t.

• It is unclear why you test for '\n' and copy the second string at the position of the newline in the first string. Strings do not end with a newline, they may contain a newline but and at a null terminator (byte value '\0' or simply 0). Strings read by fgets() may have a trailing newline just before the null terminator, but not all strings do. In your loop, the effect of copying the second string is immediately cancelled as you continue copying the bytes from the first string, even beyond its null terminator. You should perform these loops separately, first copying from t, then from s, regardless of whether either string contains newlines.

Also note that it is very bad style to declare strcaT() locally in main(), without even a proper prototype. Declare this function before the main function with its argument list.

Here is a modified version that allocates the concatenated string:

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

char *strcaT(const char *s1, const char *s2);

int main() {
    const char *a = "first";
    const char *b = "second";
    char *s = strcaT(a, b);
    if (s) {
        printf("%s\n", s);
        free(s);
    }
    return 0;
}

char *strcaT(const char *t, const char *s) {
    char *dest = malloc(strlen(t) + strlen(s) + 1);
    if (dest) {
        char *p = dest;
        /* copy the first string */
        while (*t) {
            *p++ = *t++;
        }
        /* copy the second string at the end */
        while (*s) {
            *p++ = *s++;
        }
        *p = '\0';  /* set the null terminator */
    }
    return dest;
}

Note however that this is not what the strcat function does: it copies the second string at the end of the first string, so there must be enough space after the end of the first string in its array for the second string to fit including the null terminator. The definitions for a and b in main() would be inappropriate for these semantics, you must make a an array, large enough to accommodate both strings.

Here is a modified version with this approach:

#include <stdio.h>

char *strcaT(char *s1, const char *s2);

int main() {
    char a[12] = "first";
    const char *b = "second";
    printf("%s\n", strcaT(a, b));
    return 0;
}

char *strcaT(char *t, const char *s) {
    char *p = t;
    /* find the end of the first string */
    while (*p) {
        *p++;
    }
    /* copy the second string at the end */
    while (*s) {
        *p++ = *s++;
    }
    *p = '\0';  /* set the null terminator */
    return t;
}