How might I store in a variable the string read by fgets() without leaking memory?

Question

I want the following code to accomplish to store the content of the file it reads in a variable content without leaking memory:

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

char* concat(const char *s1, const char *s2)
{
    const size_t len1 = strlen(s1);
    const size_t len2 = strlen(s2);
    char *result = malloc(len1 + len2 + 1); 
    memcpy(result, s1, len1);
    memcpy(result + len1, s2, len2 + 1); 
    return result;
}

int main() {

char path[] = "/home/jim/trackers_best.txt";
FILE *archivo = fopen(path,"r");
char line[200];
char * content = "";


if (archivo == NULL)
        exit(1);
else {

fseek(archivo, 0L, SEEK_END);
  int sz = ftell(archivo);
  fseek(archivo, 0L, SEEK_SET);
  if (sz < pow(10, 7) ) {
        printf("\nThe content of %s is:\n", path);
        while (feof(archivo) == 0) {
        fgets(line, 200, archivo); //reads one line at a time - comment out the while loop to find out
        content = concat(content, line);
       //free(content);//No leaks but no content either       
            }
          }
  else {
      puts("File take up more than 10 MBs, I refuse to read it.");
      fclose(archivo);
      exit(0);
        }
      }
fclose(archivo);
puts(content); 
free(content);
return 0;   
}

much the same as this code that uses fread() does it:

#include<stdio.h>
#include<stdlib.h>
#include <math.h>
#include <sys/types.h> 

int main() {
 
int count;  
char path[] = "/home/jim/trackers_best.txt";
FILE *archivo = fopen(path,"rb");
    
 if (archivo == NULL) {
     puts("Does not exist.");
     exit(1); 
       }
 else {
  
  fseek(archivo, 0L, SEEK_END);
  int sz = ftell(archivo);
  char contenido[sz];
  fseek(archivo, 0L, SEEK_SET);
  if (sz < pow(10, 7) ) {
    count = fread(&contenido, 10 * sizeof(char), sz, archivo);//reads 10 characters at a time.
    }
  else {
      puts("File take up more than 10 MBs, I refuse to read it.");
        }
  fclose(archivo);
  // Printing data to check validity
  printf("Data read from file: %s \n", contenido);
  printf("Elements read: %i, read %li byte at a time.\n", count, 10 * sizeof(char));
  
 }
return 0;   
}

The first code snippet leaks memory, as evidenced by valgrind:

...
total heap usage: 44 allocs, 4 frees, 23,614 bytes allocated
...
==404853== LEAK SUMMARY:
==404853==    definitely lost: 17,198 bytes in 40 blocks
...

When I attempt to prevent such leak by executing free() within the while loop I get no value stored in content. How might I store the string read by fgets without leaking memory?

Answer 1

When you call concat the second time, the value from the first call is overwritten by the malloc call. That is the memory leak.

I presume that you want content to be one long string that contains the data from all lines.

You want to use realloc (vs. malloc) in concat and just append s2 to the enlarged s1.

With this modified scheme, instead of char *content = "";, you want: char *content = NULL;

Also, don't use feof

And, ftell returns long and not int. And, it's better to not use pow to compare against the limit.

Here is the refactored code (I've compiled it but not tested it):

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

char *
concat(char *s1, const char *s2)
{
    size_t len1;
    size_t len2;

    if (s1 != NULL)
        len1 = strlen(s1);
    else
        len1 = 0;

    len2 = strlen(s2);

    s1 = realloc(s1,len1 + len2 + 1);
    if (s1 == NULL) {
        perror("realloc");
        exit(1);
    }

    memcpy(s1 + len1, s2, len2 + 1);

    return s1;
}

int
main(void)
{

    char path[] = "/home/jim/trackers_best.txt";
    FILE *archivo = fopen(path, "r");
    char line[200];
#if 0
    char *content = "";
#else
    char *content = NULL;
#endif

    if (archivo == NULL)
        exit(1);

    fseek(archivo, 0L, SEEK_END);
    long sz = ftell(archivo);
    fseek(archivo, 0L, SEEK_SET);

    if (sz < (10 * 1024 * 1024)) {
        printf("\nThe content of %s is:\n", path);

        while (fgets(line, sizeof(line), archivo) != NULL)
            content = concat(content, line);
    }
    else {
        puts("File take up more than 10 MBs, I refuse to read it.");
        fclose(archivo);
        exit(0);
    }

    fclose(archivo);

    if (content != NULL)
        puts(content);

    free(content);

    return 0;
}

Answer 2

You knew what you needed to do but didnt do it right

    content = concat(content, line);
   //free(content);//No leaks but no content either 

you need to keep the old content pointer so you can delete it after you create a new one

    char *oldc = content;
    content = concat(content, line);
    free(oldc);