What is causing this segmentation fault in string concatenation?

Question

I am doing an exercise in C to brush myself up on the topic. I am trying to use my program to manipulate header files; but the program returns "Segmentation fault". I have isolated the issue to the following segment of code. Can any one help me see what exactly is wrong with it ?

    header=strtok(st,"\"");
    header=strtok(NULL,"\"");
    f=fopen(header,"r");
    if(f)
    {
            while((ch=fgetc(f))!=EOF)
            fputc(ch,f2);

            fclose(f);
    }
    else
    {
            header2=(char *)malloc(strlen("/usr/include")+strlen(header));
            header2=strcat("/usr/include/",header);
            f=fopen(header2,"r");

            printf("%s\n",header2);

            while((ch=fgetc(f))!=EOF)
            fputc(ch,f2);

            fclose(f);
   }

I have noted that the issue occurs only when I am trying to access the /usr/include/ location. Some sort of authentication issue ? If so, how to over come it ? Thanks in advance. :)

You can't do this: `header2=strcat("/usr/include/",header);`. The first parameter has to be the destination buffer. — Eugene Sh., Jun 28 '16 at 20:30
After all the other problems are fixed, you'll still need to check the return value from the second `fopen`. — user3386109, Jun 28 '16 at 20:36
You are not opening `f2`, but re-opening `f` with a different file name (without closing the first opening). — Weather Vane, Jun 28 '16 at 20:38

score 3 · Answer 1 · answered Jun 28 '16 at 20:34

You can't use a string literal as the target of strcat -- string literals are not modifiable, and it doesn't have enough space to hold the concatenated string. A better way to combine the variable with a prefix is with sprintf.

header2 = malloc(strlen("/usr/include/")+strlen(header)+1);
sprintf(header2, "/usr/include/%s", header);

Note that you need to add 1 to the combined lengths when calling malloc(), to allow space for the trailing null byte.

You could also use strcpy followed by strcat:

strcpy(header2, "/usr/include/");
strcat(header2, header);

`sprintf` is like `printf`, but it puts the result in a string instead of printing it. — Barmar, Jun 28 '16 at 20:52

score 2 · Answer 2 · edited May 23 '17 at 11:44

The major problem is in

header2=strcat("/usr/include/",header);

as you're supplying a string literal as the destination. This does not have any space to hold the concatenated string. So, you're essentially trying to access invalid memory which invokes undefined behavior.

That said, for strcat(), the behavior is, (emphasis mine)

The strcat() function appends the src string to the dest string, overwriting the terminating null byte ('\0') at the end of dest, [...]

which, in your case, involves an attempt to modify a string literal, which again, invokes UB.

Then, there are more to point out, like

strlen() does not count the terminating null-byte for a string, so using strlen() for allocating memory should contain a +1 as the size to get the space for the null-termination.
Please see this discussion on why not to cast the return value of malloc() and family in C..
Always check the return value of library calls like fopen() for success before using the return value.

@Barmar Right, that's also a valid point, connsidered the null-substitution, let me add. — Sourav Ghosh, Jun 28 '16 at 20:32

What is causing this segmentation fault in string concatenation?

2 Answers2