strcat segmentation fault

Question

The second call to strcat here is generating a segmentation fault, why?

#include <unistd.h>
#include<stdio.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <stdlib.h>
#include <string.h>  
#include <pthread.h>

int main (int argc, char * argv[])
{
         char command[250];

         //if(scanf("%199s", command) == 1)

            gets(command);
            puts(command);

         int pipeIntId; 

         char whitespaceseparator[2]=" ";

         char pidstring [30];

         int pid= getpid(); 

         sprintf(pidstring,"%d", pid);

         char * whitespace_and_pid;

         whitespace_and_pid = strcat(whitespaceseparator,pidstring);  


         char * command_and_pid; 

         command_and_pid=strcat(command,whitespace_and_pid); // here's the problem, I guess


          if((mkfifo("pipe"/*pipeName*/,0666))==-1) 
          {
              perror("error creating pipe 1");
          exit(1);
          }

         if((pipeIntId=open("pipe",/*pipeName*/O_WRONLY))==-1)
         {
          perror("error creating pipe 2");
          exit(1);
         }


         int written;

         written=write(pipeIntId,command_and_pid,250); // send the command + the pid


         close(pipeIntId);

    return 0;
}

Answer 1

strcat is generally unsafe because it can happily overrun buffers, like it does in your case.

First of all, whitespaceseparator is only two bytes large? Are you sure that's what you want? And you concatenate pidstring to it? I think you got the arguments mixed up.

In general though, strcat will cause hard-to-debug crashes if you're not very careful with your buffer sizes. There are safer alternatives.

Answer 2

"String concatenation" is an idiom you should drop when learning C. Not only does it lead to a lot of bugs with overflowing buffers; it's also super inefficient. In your code, you could just have included the space in the snprintf format string (you should be using it in place of sprintf).

Whenever possible, try to assemble a string entirely in one step using snprintf. This consolidates all of the buffer length checking into one place and makes it really hard to get wrong. You can also call snprintf with a 0 size argument to get the length that the combined string would be, in order to find out what size to allocate, if the size of the output is not known in advance (you should allocate one more byte than this length so that the null terminator does not truncate your output).

Answer 3

whitespaceseparator isn't big enough to contain the concatenated string, so you're causing undefined behaviour.

Using gets is normally frowned upon, too.

Answer 4

I tried your code, and also see the segfault on the second strcat(). I found that command[250] is allocated immediately after whitespaceseparator[2] on the stack on my system:

(gdb) p &whitespaceseparator 
$1 = (char (*)[2]) 0xbf90acd4
(gdb) p &command
$2 = (char (*)[250]) 0xbf90acd6

e.g. (here command begins "foo..."), things are layed out like this:

 whitespaceseparator
  |
  |      command
  |       |
  v       v
+---+---+---+---+---+---+---+---+
|' '| 0 |'f'|'o'|'o'|'.'|'.'|'.'| ...
+---+---+---+---+---+---+---+---+

I can't guarantee that the same happens on your system (layout of locals on the stack may vary even between different versions of the same compiler), but it seems likely. On mine, here is exactly what happens:

As others have said, strcat() appends the second string to the first (and the result will be equal to the first argument). So, the first strcat() overflows whitespaceseparator[] (and returns whitespaceseparator as whitespace_and_pid):

+---+---+---+---+---+---+---+---+
|' '|'1'|'2'|'3'|'4'| 0 |'.'|'.'| ...
+---+---+---+---+---+---+---+---+

The second strcat() tries to append whitespace_and_pid (== whitespaceseparator) to the string at command. The first character of the copy will overwrite the terminating 0 of the string at command:

  |    ===copy===>    |
  v                   v
+---+---+---+---+---+---+---+---+
|' '|'1'|'2'|'3'|'4'|' '|'.'|'.'| ...
+---+---+---+---+---+---+---+---+

The copy continues...

      |    ===copy===>    |
      v                   v
+---+---+---+---+---+---+---+---+
|' '|'1'|'2'|'3'|'4'|' '|'1'|'.'| ...
+---+---+---+---+---+---+---+---+

          |    ===copy===>    |
          v                   v
+---+---+---+---+---+---+---+---+
|' '|'1'|'2'|'3'|'4'|' '|'1'|'2'| ...
+---+---+---+---+---+---+---+---+

and will carry on copying " 1234 1234 1234"... until it falls off the end of the process address space, at which point you get a segfault.

Answer 5

strcat doesn't do what you think. It modifies the string pointed to by its first parameter. In this case, that string is contained in a 2-byte array, which is therefore overrun.

Answer 6

To avoid buffer overflow errors, but use strcat you should to use strncat function.