Segmentation fault with strcat() in C

Question

// Processes the array, starting from the second element
    int j, k;
    char *doubled;
    for (j = 1; j < 500; j++) {
        strcpy(doubled, output[j]);
        strcat(doubled, doubled);
        for (k = 0; k < j; k++) {
            if (strcmp(output[k], output[j]) == 0) {
                output[j] = doubled;
            }
            if (strcmp(output[k], doubled) == 0) {
                output[j] = '\0';
            }
        }
    }

Trying to process an array of strings, where whenever a particular string occurs twice, the second occurrence print the duplication of the string (e.g. dog dog ---> dog dogdog), and if there are more than 2 occurrences of a string, delete the string (e.g. dog dog dog ---> dog dogdog).

I have tried to debug and I found that the problem is in this block of code, where I kept getting segmentation fault reports.

What do I have to do to fix this? I have looked at several solutions to strcat() creating segmentation faults, but it seemed none of them really worked.

When you `strcpy(doubled, output[j]);` there wan't any memory allocated to `char *doubled;` It's just an uninitialised pointer and the compiler should have warned about this. — Weather Vane, Oct 31 '19 at 23:40
I tried to use malloc() as one of the older posts suggested, but it seems like the problem is still not solved... — Curious student, Oct 31 '19 at 23:43
I tried to use memmove(doubled, doubled, strlen(doubled)); this time, but it still returns the same error report... — Curious student, Oct 31 '19 at 23:48
@HongxuZha: When you replace the strcat function call with memmove, you must adapt the first parameter of the function call, so that you are writing to the end of the string, overwriting the terminating null character of the first string. Also, strlen returns the length of the string **without** the terminating null character, so you must add 1. Therefore, you must write the following instead: `memmove( doubled + strlen(doubled), doubled, strlen(doubled) + 1 );` — Andreas Wenzel, Nov 01 '19 at 00:55

neutrino_logic · Answer 1 · 2019-11-02T05:05:17.453

Edit: As per comments, if the original array of cstrings was not allocated with enough memory per string, then probably the best solution is to construct another empty array with twice the allocated memory per string, and copy the old array into the new empty one as follows:

int count = 500;
int temp_size = 0;
char** new_array = malloc(count * sizeof(char*));
for(int i = 0; i < count; i++) {
    temp_size = strlen(output[i]);
    new_array[i] = malloc(((temp_size * 2) + 2) * sizeof(char)));
    strcpy(new_array[i], output[i]);
}

Dynamically create an array of strings with malloc

A lot of trouble can be avoided by declaring the variables inside the for loop, then their lifetime and scope is limited to the for loop. In your case, I think this works (see comments and edits):

int size = 0;
for (j = 1; j < count; j++) {
    size = strlen(output[j]);        //get the size of output[j]
    char doubled[(size * 2) + 1];    //make cstring twice size + space for '\0'
    strcpy(doubled, output[j]);      //put one copy in the temporary buffer
    strcat(doubled, output[j]);      //concatenate next copy like this
        for (k = 0; k < j; k++) {
            if (strcmp(output[k], output[j]) == 0) {
                strcpy(new_array[j], doubled);     //make changes to new_array
            }
            if (strcmp(new_array[k], doubled) == 0) {
                char deleted[1] = "\0";            //needed for free to work 
                strcpy(new_array[j], deleted);     //otherwise runtime error  
            }
        }
    }

Declaring variables inside loops, good practice or bad practice?

When done with new_array, it needs to be deleted as well:

for (int i = 0; i < count, i++) {
    free(new_array[i]);
}
free(new_array);
new_array = NULL;

You've forgotten to account for the trailing null byte and overflow the allocated buffer. The line `output[j] = doubled;` in the inner loop should cause complaints from the compiler, though I grant you it is copied from the question. Or, alternatively, the line `output[j] = '\0';` is a weird way of writing `output[j] = NULL;` or `output[j] = 0;`. It depends on the type of `output` — the size calculation suggests its an array of `char *` or a 2D array of `char`, so maybe the NULL vs `'\0'` assignment is problematic. But the variable `doubled` vanishes on each loop – ick! Pre-existing issues. — Jonathan Leffler, Nov 01 '19 at 16:05
@JonathanLeffler Thanks for the comments, I tried to fix most of the above (mainly writing a memory allocation function for copying over the array into a new, appropriately sized array with enough space). The free array function seems problematic, though. — neutrino_logic, Nov 02 '19 at 02:46

Segmentation fault with strcat() in C

1 Answers1