An element of a struct loses its value after a function is called passing another struct as argument

Question

I have a problem that I can't figure out. I have the following files: file_reader.c, file_reader.h, file_writer.c, file_writer.h, test_file_reader.c

I'm working with 'struct' to read and write files. For better understanding I wrote the following code test_file_reader.c:

#include <stdio.h>
#include "file_reader.h"
#include "file_writer.h"

int main ()
{   
    char *file_path = "/home/freitas/Dropbox/projects/gcleaner/cleaners/custom.xml";
    struct FileReader *fr = malloc(sizeof(struct FileReader));

    file_reader_new (file_path, fr);    

    show_file_reader_values(fr);

    struct FileWriter *fw = malloc(sizeof(struct FileWriter));

    fw->file_path = "/tmp/text1.txt";
    fw->content = "aaa";
    write (fw);

    show_file_reader_values(fr);

    return 0;
}

void show_file_reader_values(const struct FileReader *fr)
{
    printf("==========FILE READER==========\n");    
    printf("file path: %s\n", fr->file_path);
    printf("----------file content---------\n");    
    printf("content:\n%s\n", fr->content);
    printf("----------file content---------\n");
    printf("n lines: %d\n", fr->n_lines);
    printf("n characters: %d\n", fr->n_characters); 
    printf("==========FILE READER==========\n\n");
}

The function 'file_reader_new' reads the file and then signs the content, file path, number of lines and number of characters to the 'struct' 'FileReader'.

When I call the function 'show_file_reader_values' in the first time I do not have problems with the content but when I call the function 'write' and then call the function 'show_file_reader_values' again, the content is not the same anymore. The question is that the function 'write' of the file 'file_writer.c' and its struct does not have any relation to the file 'file_reader' and its struct. So, how can a function using another struct change the values of another struct of another file ?

The output:

[freitas@localhost test]$ ./test_file_reader 
==========FILE READER==========
file path: /home/freitas/Dropbox/projects/gcleaner/cleaners/custom.xml
----------file content---------
content:
<cleaner>  <id>k3b</id>  <label>k3b</label>  <description>Disc writing software</description>  <option>  <id>log</id>  <label>Log</label>  <description>Delete the log file which contains information about the last writing session(s).</description>  <command>delete</command>  <search>glob</search>  <path>~/.kde/share/apps/k3b/*.log</path>  </option>  <option>  <id>log2</id>  <label>Log</label>  <description>Delete the log file which contains information about the last writing session(s).</description>  <command>delete</command>  <search>glob</search>  <path>~/.kde/share/apps/k3b/*.log</path>  </option>  </cleaner>

----------file content---------
n lines: 1
n characters: 621
==========FILE READER==========

==========FILE READER==========
file path: /home/freitas/Dropbox/projects/gcleaner/cleaners/custom.xml
----------file content---------
content:
<cleaner>  <id>k��U�N
----------file content---------
n lines: 1
n characters: 621
==========FILE READER==========

Did you see ? In the first call I had the entire output:

<cleaner>  <id>k3b</id>  <label>k3b</label>  <description>Disc wri...

but in the second call I had:

<cleaner>  <id>k��U�N

file_reader.c

#include <stdio.h>
#include <stdlib.h>
#include "file_reader.h"

int file_reader_new(const char *file_path, struct FileReader *fr)
{
  char    *content;      // holds the file content.
  int     counter;           // holds the file number of lines.
  size_t  i;             // indexing into content.
  size_t  buffer_size;   // size of the content.
  char    *temp;         // for realloc().
  char    c;             // for reading from the input.
  FILE    *input;        // our input stream.

  if ((input = fopen(file_path, "r")) == NULL) {
    fprintf(stderr, "Error opening input file %s\n", file_path);
    exit(EXIT_FAILURE);
  }

  /* Initial allocation of content */
  counter = 0;
  i = 0;  
  buffer_size = BUFSIZ;
  if ((content = malloc(buffer_size)) == NULL) {
    fprintf(stderr, "Error allocating memory (before reading file).\n");
    fclose(input);
  }

  while ((c = fgetc(input)) != EOF) {

    /* Enlarge content if necessary. */
    if (i == buffer_size) {
      buffer_size += BUFSIZ;
      if ((temp = realloc(content, buffer_size)) == NULL) {
        fprintf(stderr, "Ran out of core while reading file.\n");
        fclose(input);
        free(content);
        exit(EXIT_FAILURE);
      }
      content = temp;
    }

    /* Add input char to the content. */
    content[i++] = c;

    /* If the character is a break of line
     * then the counter will be incremented.
     */
    if (c == '\n')
      counter++;
  }

  /* Test if loop terminated from error. */
  if (ferror(input)) {
    fprintf(stderr, "There was a file input error.\n");
    free(content);
    fclose(input);
    exit(EXIT_FAILURE);
  }

  /* Make the content a bona-fide string. */
  if (i == buffer_size) {
    buffer_size += 1;
    if ((temp = realloc(content, buffer_size)) == NULL) {
      fprintf(stderr, "Ran out of core (and only needed one more byte too ;_;).\n");
      fclose(input);
      free(content);
      exit(EXIT_FAILURE);
    }
    content = temp;
  }
  content[i] = '\0';

  /* Assigns the variables to the corresponding
   * element of the struct.
   */
  fr->file_path = file_path;
  fr->content = content;
  fr->n_lines = counter;
  fr->n_characters = i;

  /* Clean up. */
  free(content);
  fclose(input);

  return 0;
}

file_reader.h

#ifndef FILE_READER_H_
#define FILE_READER_H_

typedef struct FileReader
{
    char *content;           // holds the file content.
    char *file_path;     // holds the file path.
    int  *n_lines;           // holds the number of lines.
    int  *n_characters;  // holds the number of characters.
} FileReader;

// file_reader_new - reads the file
int file_reader_new(const char *file_path, struct FileReader *fr);

#endif

file_writer.c

#include <stdio.h>
#include "file_writer.h"

void write (struct FileWriter *fw)
{
  FILE *f = fopen(fw->file_path, "w");
  if (f == NULL)
  {
      printf("Error opening file!\n");
      exit(1);
  }

  fprintf(f, "%s", fw->content);   

  fclose(f);
}

file_writer.h

#ifndef FILE_WRITER_H_
#define FILE_WRITER_H_

typedef struct FileWriter
{
    char *file_path;
    char *content;
    int  *error;
} FileWriter;

#endif

Can you help me ? Thanks!

Answer 1

I am not sure how are you reading from the file, character by character or block, but anyhow ,

since you update the read data in content buffer, and store the address of content buffer inside file_reader_new() into variable fr->content and immediately releasing the memory will end up loosing the data you read. and lead to condition called Dangling pointer

Dangling pointer

( a pointer variable, which points to a released memory )

that's why its always advised to set the pointer variable after releasing to NULL. Dereferencing a dangling pointer is will lead to Segmentation fault or undefined behavior in some scenarios.

Also, since all you member variables of struct are pointers its better to initialize them to NULL.

you can use calloc to initialize all the variables in a struct, instead of malloc to initialize all the members to NULL, if you are going with dynamic allocation. which goes for string also.

Answer 2

struct FileReader *fr = malloc(sizeof(struct FileReader)); 

There is no need to do this. All you need is this:

struct FileReader fr; 

Same here:

struct FileWriter fw; 

Then just pass the address of these variables to the requisite function(s).

---

Note this was not given to you as an answer, only as a comment to clean up your code a bit to remove extraneous calls to the heap. It just so happens that the real problem exists elsewhere, and what you're seeing here is undefined behavior in full glory.

Answer 3

Here is an issue that I see:

fr->content = content;
fr->n_lines = counter;
fr->n_characters = i;

/* Clean up. */
free(content);   /* <-- Danger */

You do this in your file_reader_new function. You then call show_file_reader_values and in that function, you're accessing content:

printf("content:\n%s\n", fr->content);

Since you called free() on the content, that pointer no longer points to valid memory, thus undefined behavior occurs.

The fix is to allocate space on fr for the content and copy the characters of content to this space, or simply not call free on content.

So either do this:

fr->content = malloc(i + 1);
strcpy(fr->content, content);
fr->n_lines = counter;
fr->n_characters = i;

/* Clean up. */
free(content);   

or this:

fr->content = content;
fr->n_lines = counter;
fr->n_characters = i;
/* No call to free(content) done */