binary file read/write error containing char* pointer within struct

Question

I have a strange problem. I can not guess why it is happening. I tried in various ways. May be it is because still I am newbie for c language.

Please look at the below code.

It comes with 2 arguments. --write and --read.

• In my write() function I write to the file and then I call the read() function. This write the data to file and print 3 lines of values correctly as expected.

• In my read() function I read the file. When I pass the --read argument alone , the program gives segmentation fault error message. Although in below code if I assign the static string value to char *name this read function works as expected.

Below is my full code that I created to simulate my issue.

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

typedef struct _student {
    int id;
    char *name;
} Student;

void write();
void read();

int main(int argc, char *argv[])
{
    if (argc > 1) {
        if (strcmp(argv[1], "--write") == 0) {
            write();
            read();
        }
        else if (strcmp(argv[1], "--read") == 0) {
            read();
        }
    }
    return 0;
}

void write()
{
    printf("Write\n");
    FILE *fp;

    // write student
    Student *std_writer = (Student *) malloc(sizeof(Student));
    std_writer->id = 10;
    //std_writer->name = "Alice"; // But if i remove the below 4 lines and uncommented this line, everything works as expected.
    char *a = "Alice";
    std_writer->name = malloc(20);
    memset(std_writer->name, '\0', 20);
    strncpy(std_writer->name, a, 5);

    fp = fopen("Student.file", "wb");
    fwrite(std_writer, sizeof(Student), 1, fp);
    fwrite(std_writer, sizeof(Student), 1, fp);
    fwrite(std_writer, sizeof(Student), 1, fp);
    fclose(fp);

    free(std_writer);
}

void read()
{
    printf("Read\n");
    FILE *fp;

    // read student
    Student *std_reader = (Student *) malloc(sizeof(Student));
    fp = fopen("Student.file", "rb");
    while(fread(std_reader, sizeof(Student), 1, fp) == 1) {
        printf("ID %i  \tName : %s\n", std_reader->id, std_reader->name);
    }
    fclose(fp);

    free(std_reader);
}

please help me understand and solve this issue.

EDIT

Ok According to the below answers as I understood, I udpated my struct Student as follows.

typedef struct _student {
    int id;
    char name[20];
} Student;

This works.

Any comments ?

Answer 1

Don't call your functions read and write (these names are for Posix functions). And don't expect to be able to read again a pointer written by a different process. This is undefined behavior.

So in your write you are (assuming a 64 bits x86 system, e.g. a Linux one) writing 12 bytes (4 i.e. sizeof(int) + 8 i.e. sizeof(char*)); the last 8 bytes are the numerical value of some malloc-ed pointer.

In your read your are reading these 12 bytes. So you are setting the name field to the numerical pointer which happened to be valid in the process which has done the write. This won't work in general (e.g. because of ASLR).

In general, doing I/O on pointers is very bad. It has only sense for the same process.

What you want to do is called serialization. For software engineering reasons I recommend using textual format for serialization (e.g. JSON, perhaps using the Jansson library). Textual formats are less brittle, and easier to debug.

---

Assuming you would encode a student in JSON format like

{ "id":123, "name":"John Doe" }

here is a possible JSON encoding routine using Jansson:

int encode_student (FILE*fil, const Student*stu) {
   json_t* js = json_pack ("{siss}", 
                           "id", stu->id, 
                           "name", stu->name);
   int fail = json_dumpf (js, fil, JSON_INDENT(1));
   if (!fail) putc('\n', fil);
   json_decref (js); // will free the JSON
   return fail;  
}

Notice that you need a function to free a malloc-ed Student zone, here it is:

void destroy_student(Student*st) {
   if (!st) return;
   free (st->name);
   free (st);
}

and you might want also the macro

#define DESTROY_CLEAR_STUDENT(st) do \
  { destroy_student(st); st = NULL; } while(0)

Now, here is the JSON decoding routine using Jansson; it gives a Student pointer in heap (to be later destroyed by the caller with DESTROY_CLEAR_STUDENT).

Student* decode_student(FILE* fil) { 
   json_error_t jerr;
   memset (&jerr, 0, sizeof(jerr));
   json_t *js = json_loadf(fil, JSON_DISABLE_EOF_CHECK, &err);
   if (!js) {
      fprintf(stderr, "failed to decode student: %s\n", err.text);
      return NULL;
   }
   char* namestr=NULL;
   int idnum=0;
   if (json_unpack(js, "{siss}",  
                       "id", &idnum,
                       "name", &namestr)) {
       fprintf(stderr, "failed to unpack student\n");
       return NULL;
   };
   Student* res = malloc (sizeof(Student));
   if (!res) { perror("malloc student"); return NULL; };
   char *name = strdup(namestr);
   if (!name) { perror("strdup name"); free (res); return NULL; };
   memset(res, 9, sizeof(Student));
   res->id = id;
   res->name = name;
   json_decref(js);
   return res;
}

---

You could also decide that you serialize in some binary format (I don't recommend doing that). Then you should define your serialization format and stick to it. Very probably you'll have to encode the student id, the length of the its name, its name....

You could also (in C99) decide that the student's name is a flexible array member, that is declare

typedef struct _student {
   int id;
   char name[]; // flexible member array, conventionally \0 terminated
} Student;

---

You really want the student names to be of varying length. You then cannot simply put varying length records in a simple FILE. You could use some indexed file library like GDBM (each record could be in JSON). And you probably want to use Sqlite or a real database like MariaDb or MongoDB.

Answer 2

Notice that you are not writing the name of the student to file. Your are only writing the pointer to that string. This, for sure, is not what you want. When you read the file you are reading the pointer which is no longer valid.

Either put the entire string in your struct (not a char pointer but a char array) or else you should separately write the strings to file.

Answer 3

In read(), you don't ever allocate memory for name in your Student structure. (Your write() function is much better behaved in that respect.)

When you refer to it in you printf statement, you invoke undefined behaviour.