C - segmentation core dump on strcpy()

Question

I get a segmentation fault each time I'm trying to run this function.

char *hist_array[20];
int history_counter = 0;

void save_to_history(char *temp){
    temp = malloc(512);/*512 is the size of temp array*/
    printf("temp = %s\narray = %s",temp,hist_array[history_counter]);/*debug*/
    strcpy(hist_array[history_counter],temp);
    printf("Saved %s to history to %d\n\n",hist_array[history_counter],history_counter);
    history_counter++;
}

I'm not sure whether I'm using malloc correctly or not, but as I understand it should help with properly saving my string temp to an array of strings hist_array. Also, temp is never NULL.

EDIT 1: Changed sizeof(temp) to its proper size 512, still getting segfault.

Nope, this won't work. sizeof(temp) is the size of the pointer, not the passed in string. You need to pass in the string size as a parameter. — OldProgrammer, Mar 23 '14 at 15:55
You also appear to be overwriting the pointer which was passed in with a pointer to a new uninitialized block of memory - then using it as a source for a strcpy. — Baldrick, Mar 23 '14 at 15:56
@OldProgrammer @Baldrick I've changed the `sizeof(temp)` to it's proper value, still getting segfault. @Baldrick so I need to initialize the array `hist_array`? — Vitalij Kornijenko, Mar 23 '14 at 16:04
did you allocated memory for pointers in `hist_array` somewhere? If not - that is the reason for segfault. — Ryzhehvost, Mar 23 '14 at 16:07

score 3 · Accepted Answer · answered Mar 23 '14 at 16:04

The problem is with the following statement -

strcpy(hist_array[history_counter], temp);

strcpy tries to copy the buffer pointed to by temp to the buffer pointed to by hist_array[history_counter] but char *hist_array[20]; defines hist_array to be an array of 20 pointers to characters. You should change your function to -

char *hist_array[20];
int history_counter = 0;

void save_to_history(char *temp) {
    // +1 for the terminating null byte as strlen
    // does not count the null byte

    hist_array[history_counter] = malloc(1 + strlen(temp)); 
    if(hist_array[history_counter] == NULL) {
        printf("not enough memory to allocate\n");
        // handle it
    }
    strcpy(hist_array[history_counter], temp);
    printf("Saved %s to history to %d\n\n",
           hist_array[history_counter], 
           history_counter);

    history_counter++;
}

score 2 · Answer 2 · answered Mar 23 '14 at 16:05

In your code, you are allocating via malloc but you don't initialize it. So when you do this:

printf("temp = %s\narray = %s",temp,hist_array[history_counter]);/*debug*/
strcpy(hist_array[history_counter],temp);

The should print some garbage and the subsequent strcpy will copy whatever is in temp at the time, until it encounters a 0 byte. You are invoking undefined behaviour here, because you should initialize the memory.

For example you could do:

memset(temp, 0, 512);

Additionally you are dereferencing pointers in your array hist_array. Since this is a global variable, all the pointers will originally be NULL pointers, which is causing your segfaul there.

C - segmentation core dump on strcpy()

2 Answers2