Segmentation fault 11 printing string from struct

Question

This is the first time I've run into Segmentation fault 11 in C and I can't seem to wrap my head around what is actually going wrong.

What I'm trying to do is write a few int values to a struct plus the file name from the command line (char *) from a child process and then write the struct to the pipe to read from from the parent process. It works fine when it's only the integers and I take out the code working with the string, but once I add in the string and try to print out the file name in the parent process I get the segmentation fault 11 when the program is run.

I've looked at various posts from all over, but have noticed that the common issue for this is when someone attempts to assign a string to a char array and prints, but I made sure to use only char * here. Here's the code where it locks up

 if((read(pd[0], &pv, 2048)) == -1)
 {
   error_exit("read not working");
 }

 printf("words = %d\n", pv.words);
 printf("lines = %d\n", pv.lines);
 printf("bytes = %d\n", pv.bytes);
 printf("file = %s\n", pv.file); //locks up here and gives segmentation fault 11 on the command line

Here is the read out of what the program does when I run it:

$ ./a testfile
Parent process... should be waiting on child...
In child process! pid = 21993  
it worked? testfile
Done with child process!
words = 1
lines = 2
bytes = 3
Segmentation fault: 11

Also here is the full code EDIT: I swapped out the code using sizeof for string and used strlen

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>

void error_exit(char *);

typedef struct total {

  int words, lines, bytes;
  char *file;

} Vals;

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

  int pd[2]; //pipe descriptor
  pid_t pid;
  Vals v, pv;
  char *fname = "Not set";

  if(argc > 1)
  {
    fname = malloc(strlen(argv[1]));
    strcpy(fname, argv[1]);
  }

  if((pipe(pd)) == -1)
  {
    error_exit("pipe creation");
  }

  if((pid = fork()) == -1)
  { 
    error_exit("the fork forked up!");
  }
  else if(pid == 0)
  {

    printf("In child process! pid = %d\n", getpid());
    v.words = 1;
    v.lines = 2;
    v.bytes = 3;
    v.file = malloc(strlen(fname));
    strcpy(v.file, fname);
    printf("it worked? %s\n", v.file);

    close(pd[0]);

    if((write(pd[1], &v, sizeof(v.words) + sizeof(v.lines) + sizeof(v.bytes) + strlen(v.file))) == -1)
    {
      error_exit("Write from child");
    }

    //return; //return from child
    printf("Done with child process!\n");
    close(pd[1]);
    return 0;
  }
  else
  {
    printf("Parent process... should be waiting on child...\n");
  }
  //wait for child
  while((pid = wait(NULL)) > 0);

  close(pd[1]);

  //Vals pv = {0, 0, 0, "pv.file not set"};

  //just assign anything to file to see if it fixes
  //pv.file = malloc(strlen(fname));

  if((read(pd[0], &pv, 2048)) == -1)
  {
    error_exit("read not working");
  }

  printf("words = %d\n", pv.words);
  printf("lines = %d\n", pv.lines);
  printf("bytes = %d\n", pv.bytes);
  printf("file = %s\n", pv.file); //locks up here and gives segmentation fault 11 on the command line

  close(pd[0]);

  //program ended normally
  return 0;

}

void error_exit(char *err)
{
  printf("exiting because of this section: %s\nerrno = %d", err, errno);
  exit(1);
}

I really appreciate any insight on this!

Answer 1

This code has several issues which for some reason were not mentioned. Hence here goes my take.

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>

Well, gcc -Wall -Wextra tells me:

warning: implicit declaration of function ‘wait’

How are you compiling this? Did you see this error and ignored it? If so, no candy for a week.

void error_exit(char *);

typedef struct total {

  int words, lines, bytes;
  char *file;

} Vals;

Weird naming. 'total'? 'vals'?

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

  int pd[2]; //pipe descriptor

Rather useless comment.

  pid_t pid;
  Vals v, pv;
  char *fname = "Not set";

  if(argc > 1)

Should test argc == 2 and throw insults if > 2.

  {
    fname = malloc(strlen(argv[1]));
    strcpy(fname, argv[1]);

Incorrect. strlen return the length without the terminating null character. Consider using strdup instead (non-standard). Missing NULL check.

  }

  if((pipe(pd)) == -1)
  {
    error_exit("pipe creation");
  }

  if((pid = fork()) == -1)
  { 
    error_exit("the fork forked up!");
  }
  else if(pid == 0)
  {

    printf("In child process! pid = %d\n", getpid());
    v.words = 1;
    v.lines = 2;
    v.bytes = 3;
    v.file = malloc(strlen(fname));
    strcpy(v.file, fname);
    printf("it worked? %s\n", v.file);

    close(pd[0]);

You typically close earlier.

    if((write(pd[1], &v, sizeof(v.words) + sizeof(v.lines) + sizeof(v.bytes) + strlen(v.file))) == -1)
    {
      error_exit("Write from child");
    }

This code this does not work, but you may be tempted to use 'char file[BIGNUM];' mentioned in other comments, so let's steal a sample which was supposed to work:

    if((write(pd[1], &v, sizeof(v.words) + sizeof(v.lines) + sizeof(v.bytes) + sizeof(v.file) + 1)) == -1) {
        error_exit("Write from child");
    }

Incorrect. Let's assume this adds up to the size of the structure - then '+1' found here causes reading 1 byte after the structure. But sizes of all struct elements are not guaranteed to add up to the size of the entire structure due to padding. If using 'char file[BIGNUM];' just sizeof(v). If playing with char *file, you have to make sure file is always last and for simplicity just use offsetof to the file pointer.

    //return; //return from child
    printf("Done with child process!\n");
    close(pd[1]);
    return 0;

Incorrect. Should use _Exit(2) instead.

  }
  else
  {
    printf("Parent process... should be waiting on child...\n");
  }

What's up with the else clause which only prints something and passes execution below?

  //wait for child
  while((pid = wait(NULL)) > 0);

Incorrect. wait can return due to a signal.

  close(pd[1]);

Should close before wait.

  //Vals pv = {0, 0, 0, "pv.file not set"};

  //just assign anything to file to see if it fixes
  //pv.file = malloc(strlen(fname));

  if((read(pd[0], &pv, 2048)) == -1)
  {
    error_exit("read not working");
  }

pv does not have 2048 bytes, so this can happen to work only by accident.

  printf("words = %d\n", pv.words);
  printf("lines = %d\n", pv.lines);
  printf("bytes = %d\n", pv.bytes);
  printf("file = %s\n", pv.file); //locks up here and gives segmentation fault 11 on the command line

  close(pd[0]);

  //program ended normally
  return 0;

}

void error_exit(char *err)
{
  printf("exiting because of this section: %s\nerrno = %d", err, errno);
  exit(1);
}

Consider using perror or err-family of functions (not portable).

Finally, I recommend finding less atrocious style (from linux or KNF).

Answer 2

There's a few things wrong here. First, you aren't free()ing the space you allocate with malloc().

Second, you should be using strlen() in place of sizeof() in your calculations. This occurs twice in your code.

Third, the declaration char fname = "Not set"; is not safe, since it is actually a const char* to read-only memory (text segment), and it's later pointed to something allocated via malloc(). Don't do this.

Corrected Code Listing

---

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <string.h>
#include <errno.h>

#define MAX_BUF_LEN (1024)

void error_exit(char *);

typedef struct total {

    int words, lines, bytes;
    char file[MAX_BUF_LEN];

} Vals;

int main(int argc, char *argv[])
{
    int pd[2]; //pipe descriptor
    pid_t pid;
    Vals v, pv;
    char fname[MAX_BUF_LEN] = "Not set";

    if(argc > 1) {
        //fname = malloc(sizeof(argv[1]) + 1);
        //fname = argv[1];
        strcpy(fname, argv[1]);
    }

    if((pipe(pd)) == -1) {
        error_exit("pipe creation");
    }

    if((pid = fork()) == -1) { 
        error_exit("the fork forked up!");
    } else if(pid == 0) {
        printf("In child process! pid = %d\n", getpid());
        v.words = 1;
        v.lines = 2;
        v.bytes = 3;
        //v.file = malloc(strlen(fname) + 1);
        strcpy(v.file, fname);
        printf("it worked? %s\n", v.file);
        close(pd[0]);

        if((write(pd[1], &v, sizeof(v.words) + sizeof(v.lines) + sizeof(v.bytes) + sizeof(v.file) + 1)) == -1) {
            error_exit("Write from child");
        }

        printf("Done with child process!\n");
        close(pd[1]);
        return 0; //return from child
    }
    else
    {
        printf("Parent process... should be waiting on child...\n");
    }
    //wait for child
    while((pid = wait(NULL)) > 0);

    close(pd[1]);

    if((read(pd[0], &pv, 2048)) == -1) {
        error_exit("read not working");
    }

    printf("words = %d\n", pv.words);
    printf("lines = %d\n", pv.lines);
    printf("bytes = %d\n", pv.bytes);
    printf("file = %s\n", pv.file); //locks up here and gives segmentation fault 11 on the command line

    close(pd[0]);

    //program ended normally
    return 0;

}

void error_exit(char *err)
{
    printf("exiting because of this section: %s\nerrno = %d", err, errno);
    exit(1);
}

Sample Run

---

Parent process... should be waiting on child...
In child process! pid = 7410
it worked? HelloWorld
Done with child process!
words = 1
lines = 2
bytes = 3
file = HelloWorld

Answer 3

Your main problem is that you don't quite have the right understanding of C strings. You cannot do sizeof(char_pointer). That will just give you the pointer size (4 in a 32 bit system) and not the size of the string it points to. Use strlen to get the length of a string.

The second related problem is that you are writing a pointer address, v.file, and not the full string contents through the pipe. That is not correct because each process has a seperate address space and hence a pointer in one process is not valid in another process.

There are several ways to fix your problem. I will give you the simplest (but not the best).

First declare file inside the struct as a char array rather than a char pointer. This essentially gives you a fixed sized buffer.

#define MAX_FILENAME_LEN 64
typedef struct total {
  int words, lines, bytes;
  char file[MAX_FILENAME_LEN];
} Vals;

Then remove the malloc call. You don't need it anymore as file is already a buffer that you can copy into.

Finally, make sure you don't overflow the buffer during string copy:

if (strlen(fname) >= MAX_FILENAME_LEN) {
    error_exit("File name too long");
}
strcpy(v.file, fname);

You also don't need the +1 in the write as the sizeof gives you the full buffer size.

I'll leave it as an exercise for you to use dynamic memory for the file name in the struct. It's not hard but will require you to change your read and write logic a little as you will need to read/write the file name seperately (because writing the whole struct in that case will just write the pointer not the contents).