How to Dynamically Allocate a string within a struct with fscanf or fgets?

Question

I'm trying to allocate a string within a struct with fscanf, I tried this:

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

typedef struct _SPerson {
  char *name;
  char *surname;
  char *id;
  char *telephone;
}SPerson;

void main (void) {
  unsigned int ne;
  SPerson Archive[1000];
  Load(Archive,&ne);
}

int Load(SPerson Archive[],unsigned int *ne) {
  int k,i=0;
  char s[4][20];
  FILE *f;
  f = fopen("archive.txt","r");
  if(f==0) return 0;

  while((k=fscanf(f,"%s %s %s %s",s[0],s[1],s[2],s[3]))==4) {
    Archive[i].id = (char*) malloc( sizeof(char) *strlen(s[0])); 
    Archive[i].id =s[0];
    Archive[i].name = (char*) malloc( sizeof(char) *strlen(s[1])); 
    Archive[i].name = s[1];
    Archive[i].surname = (char*) malloc( sizeof(char) *strlen(s[2])); 
    Archive[i].surname = s[2];
    Archive[i].telephone = (char*) malloc( sizeof(char) *strlen(s[3])); 
    Archive[i].telephone =s[3];
    i++;    
  }

  *ne = i;
  fclose(f);
  return 1;
}

Maybe in my brain it's correct, but something went wrong while loading the data, so is this the correct and clear method to read strings dynamically? I thought to use fgets, but my string is separated by a space, so I need to implement another function, the split. Can Anyone help me?

Never use the `scanf` functions, they are broken as designed. You want `fgets` + `strtok` + `strdup`. Also, `sizeof(char)` is 1 *by definition* so never write it. — zwol, Jan 08 '16 at 03:59
Example of your input file showing how it is delimited would help. Just a few lines, if all lines contain identical sets of fields. — ryyker, Jan 08 '16 at 04:15
All your memory allocations are 1 byte too short (you need to allocate `strlen(name)+1` bytes). You then leak the memory by using assignment instead of `strcpy()`. Further, you continually overwrite the same variables, so all the data ends up looking like the last row read. You've not ensured no buffer overflow with the format strings. You should check whether your version of [`scanf()`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/scanf.html) is POSIX-like enough to support `%ms`, which has `scanf()` allocate the memory for you — but you pass a `char **` instead of a `char *`. — Jonathan Leffler, Jan 08 '16 at 04:37
Also `Archive[i].name = s[1];` isn't doing what you think it is... — John Hascall, Jan 08 '16 at 04:54
`Archive[i].id = (char*) malloc( sizeof(char) *strlen(s[0])); Archive[i].id =s[0];` --> `Archive[i].id = (char*) malloc( sizeof(char) *(strlen(s[0])+1)); strcpy(Archive[i].id, s[0]);` — BLUEPIXY, Jan 08 '16 at 07:25

score 2 · Accepted Answer · edited Jan 08 '16 at 04:58

 while((k=fscanf(f,"%s %s %s %s",s[0],s[1],s[2],s[3]))==4) {
     //your code
     i++;    
 }

Instead of this use fgets to read complete data and then tokenize it using strtok -

char data[1024],*token;
 int j=0;
while(fgets(data,sizeof data,f)!=NULL){         //read from file
       token=strtok(data," ");                  //tokenize data using space as delimiter
       while(token!=NULL && j<4){
            j=0;
            sprintf(s[j],"%s",token);          //store it into s[i]
            j++;
            token=strtok(NULL," ");
       }
       Archive[i].id = malloc(sizeof *Archive[i].id * (strlen(s[0])+1));    //allocate memory 
       strcpy(Archive[i].id,s[i]);          //copy at that allocated memory
         //similar for all
       i++;
}

This can be used instead of your loop.

Note - Do not do this -

 Archive[i].id = (char*) malloc( sizeof(char) *(strlen(s[0])+1)); 
 Archive[i].id =s[0];         <-- 2.

As after 2. statement you will lose reference to previous allocated memory and will not be able to free it — causing a memory leak.

This is same for all such following statements.

score 1 · Answer 2 · edited May 23 '17 at 11:59

A couple of quick suggestions to improve:

1) void main (void) is really not a good prototype for main. Use:

int main(void);

Or:

int main(int argc, char **argv);

2) There is no need to cast the return of [m][c][re]alloc in C.
This line in your code:

Archive[i].id = (char*) malloc( sizeof(char) *strlen(s[0]));
                ^^^^^^^         ^^^^^^^^^^^^^^              //^^^ == remove

Should be written:

Archive[i].id = malloc( strlen(s[0]) + 1);//no cast, and sizeof(char) is always == 1
                                          //"+ 1" for NULL termination

3) Suggest using fgets(), strtok() and strcpy() as a minimal method to read, parse and copy strings from file into struct members:

Note: there will be a number of calls to malloc() here, and each will have to be freed at some point. To avoid all that, it would be better if your struct contained members with hard coded stack memory:

typedef struct
{
    char name[80];
    char surname[80];
    char id[80];
    char telephone[80];
}SPerson;

But assuming you have a reason to use heap memory, here is a way to do it using the struct as you have defined it:

char line[260];//line buffer (hardcoded length for quick demo)
char *tok={0};//for use with strtok()
int len = 0;//for use with strlen()
FILE *f;

f = fopen("archive.txt","r");//did not have example file for this demo
                             //therefore do not have delimiters, will guess
if(f==0) return 0;

i = 0;//initialize your index
while(fgets(line, 260, f))
{
    tok = strtok(line, " ,\n\t");// will tokenize on space, newline, tab and comma
    if(tok)
    {
        len = strlen(tok);
        Archive[i].id = malloc(len + 1);//include space for NULL termination
        strcpy(Archive[i].id, tok);//note correct way to assign string
        //(Archive[i].id = tok is incorrect!!)
    }
    else {//handle error, free memory and return}
    tok = strtok(NULL, " ,\n\t");//note NULL in first arg this time 
    if(tok)
    {
        len = strlen(tok);
        Archive[i].name= malloc(len + 1);//include space for NULL termination
        strcpy(Archive[i].name, tok);

    }
    else {//handle error, free memory and return}
    //...And so on for rest of member assignments
    //
    i++;//increment index at bottom of while loop just before reading new line
}

How to Dynamically Allocate a string within a struct with fscanf or fgets?

2 Answers2