Segmentation fault in my strcpy

Question

I'm writing my own strcpy due to the fact that the default one in string.h only accept a const char * as a source string to copy from. I'm trying this very basic prototype (yes, the return isn't meaningful, I'm just trying things):

int copyStrings(char * dest, char * source){
    int i=0;
    while(source[i]!='\0'){
        dest[i]=source[i];
        i++;
    }
    dest[i]='\0';
    return 0;
}

and it gives me SIGSEGV, Segmentation Fault error in gdb, at the line dest[i]=source[i], right at the first character. I'm pretty sure dest[i] isn't a string literal, so I should be able to modify it.

What am I doing wrong?

EDIT: here's the calling

int main(){
    char * str = (char*)malloc((int)sizeof(double));
    char * str2 = (char *)malloc((int)sizeof(int));
    str = "hello";
    str2 = "hey jude";
    copyStrings(str2, str);
    free(str);
    free(str2);
    return 0;
}

Show the caller. BTW, it should return something meaningful. — Eugene Sh., Jul 18 '18 at 14:50
What's the problem with `strcpy()` only accepting a `const char *`? You can pass a non-const `char *` to the function and the function simply promises it won't change the data via that `const char *`. It does no harm; you can pass const or non-const strings as the second argument. — Jonathan Leffler, Jul 18 '18 at 14:52
What the size of `int` and `double` have to do with strings? Also you assign both of them with string literals afterwards. — Eugene Sh., Jul 18 '18 at 14:56
@EugeneSh. I'm trying to make sure that they both have enough space to store chars. — riciloma, Jul 18 '18 at 14:58
Since `malloc()` expects a `size_t` argument, your `(int)` casts on the result of `sizeof` are odd. The string `"hey jude"` is 9 bytes long; on most machines, `sizeof(double)` is `8` (and `sizeof(int)` is usually `4`). That's a probable cause of trouble. You're leaking memory; and you **ARE** trying to assign to constants (string literals). You need to use `strcpy()` to assign `"hello"` and `"hey jude"` — and you need to make sure you've got enough space to copy the strings into. Hint: you've not allocated enough space. Change your `sizeof()` operands to `16`. Use `strcpy()` to copy. — Jonathan Leffler, Jul 18 '18 at 14:58
Assigning string literals to pointers the way you do here (probably) does not do what you think it does. — 500 - Internal Server Error, Jul 18 '18 at 14:58
If you could assign strings with `=` you wouldn't need `strcpy`. — Eugene Sh., Jul 18 '18 at 14:59
Do you understand what `const char *` means in this case, and why it's a Good Thing? — Bob Jarvis - Слава Україні, Jul 18 '18 at 14:59
@BobJarvis isn't it telling me that it wants a string literal in the calling? — riciloma, Jul 18 '18 at 15:00
No: the `const char *` argument says "I, `strcpy()`, promise not to try to modify the string passed as my second argument; I'll read it, but I won't write to it". — Jonathan Leffler, Jul 18 '18 at 15:01
No. The prototype for `strcpy` is `char * strcpy ( char * destination, const char * source)`. In this case `const char * source` means that `strcpy` is not allowed to modify the memory pointed to by the `source` parameter. It does not mean that `source` must be a constant, nor does it mean that `source` must be unmodifiable - it only means that `strcpy` promises that it won't change `source`. Thinking about this differently - if your contention was correct that `strcpy` can only copy *string constants* around, it would be a pretty useless routine, wouldn't it? Well - best of luck. — Bob Jarvis - Слава Україні, Jul 18 '18 at 15:09
Got it. Thank you everyone. I shall use strcpy and research more into string literals. — riciloma, Jul 18 '18 at 15:27

score 3 · Accepted Answer · edited Jul 18 '18 at 15:04

3

This is assigning a string literal to str2 - the very thing that you claim you aren't doing. This is actually the cause of your segfault.

str2 = "hey jude";

It also is causing a memory leak as prior to this, you malloc'd some memory and assigned it to str2 as well. But not enough memory to hold the string. Typically an int is 4 bytes and you need 9 bytes to store that string.

What you want to do is this, which allocates as many bytes as there are in the string, plus an extra one to store the \0 terminating character at the end.

str2 = malloc(strlen("hey jude")+1);
strcpy(str2,"hey jude");

or on some systems you can use POSIX function strdup() which effectively does the job of the above in one handy function call.

str2 = strdup("hey jude");

edited Jul 18 '18 at 15:04

Jonathan Leffler

730,956
141
904
1,278

answered Jul 18 '18 at 15:03

Chris Turner

8,082
1
14
18

Strictly, of course, you should check the result of `malloc()`. It isn't critical just at the moment — there are other bigger problems in the original code. – Jonathan Leffler Jul 18 '18 at 15:05
Writing to read-only memory will cause a SIGSEV. That's why you get segmentation fault on the first line of your `copyString` function : you assign to `str2` the address of a litteral const, and then you try to copy `str` over it. – MartinVeronneau Jul 18 '18 at 15:07
Would this explain why if i do `free(str2)` after doing `str2 = "hey jude";` gives me segfault? Because I've assigned a literal to it? – riciloma Jul 18 '18 at 15:08
@riciloma yes, plus it wouldn't be freeing the memory you've allocated as you've already overwritten it – Chris Turner Jul 18 '18 at 15:09

score 0 · Answer 2 · answered Jul 18 '18 at 15:29

Let's go at it line by line and see where it goes wrong:

int main(){
    char * str = (char*)malloc((int)sizeof(double));
    char * str2 = (char *)malloc((int)sizeof(int));
    str = "hello";
    str2 = "hey jude";
    copyStrings(str2, str);
    free(str);
    free(str2);
    return 0;
}

int main(){ - this is an improper definition of main. Should be int main(int argc, char **argv)

char * str = (char*)malloc((int)sizeof(double)); - defines str, then allocates (probably) 8 bytes of memory and assigns its address to str. malloc takes a size_t argument, so the cast (int)sizeof(double) is incorrect. Also, in C the return value of malloc should never be cast. So this line should be char * str = malloc(sizeof(double));

char * str2 = (char *)malloc((int)sizeof(int)); - all the same problems as the preceding line. Should be char *str2 = malloc(sizeof(int));

str = "hello"; - causes a memory leak, because the memory you JUST ALLOCATED two lines earlier is now irretrievably lost. You've got two options here - either don't allocate the memory when defining str or free it first. Let's do the latter:

free(str);
str = "hello";

str2 = "hey jude"; - same problem, similar solution:

free(str2);
str2 = "hey jude";

copyStrings(str2, str); - here you're telling your routine to copy the constant string "hello" over the top of the constant string "hey jude". This will work fine on some systems, but will blow up on other systems. The question is in the treatment of the constant string "hey jude". If it's stored in modifiable memory the code will work just fine. If it's stored in memory which is marked as being unmodifiable, however, it will blow up. It seems that the latter is the case on your system. To fix this you probably want to go back to the previous line and change it to

str2 = malloc(20);

That's more memory than you'll need, but it will work just fine.

free(str); - you're attempting to free the constant string "hello", which is not dynamically allocated memory. This needed to be done prior to the assignment str = "hello";.

free(str2; - same problem as above. This needed to be done prior to the assignment str2 = "hey jude";.

} - correct

Best of luck.

Segmentation fault in my strcpy

2 Answers2