Every pointer in an array of pointers store the same value after reading in from a file in C

Question

Here is code snippet where I read values from a file and createClient from that data. I have an array of Clients and I am giving different values to different elements of array.

FILE *clientFile;
clientFile = fopen("clients.txt", "r");
char id[256];
char name[256]; 
char phone[256];
char email[256];
Client cptrs[10];
int i=0;
while(fgets(id, sizeof(id), clientFile)){
    //fscanf(clientFile, "%s", name);
    fgets(name, sizeof(name), clientFile);
    fgets(phone, sizeof(phone), clientFile);
    fgets(email, sizeof(email), clientFile);
    /*fscanf(clientFile, "%s", phone);
    fscanf(clientFile, "%s", email);*/
    //printf("%s %s %s %s\n", id,name,phone,email);
    cptrs[i] = createClient(id,name,phone,email);
    //printc(cptrs[i]);
    //printf("%d\n", i);
    i++;

}
printc(cptrs[0]);
printc(cptrs[1]);
printc(cptrs[2]);

All 3 print functions output the same result which is the last data in the file.

Here are struct client, createClient method and printc method. I have included both client.h and client.c files.

client.h

#ifndef CLIENT_H
#define CLIENT_H

typedef struct client *Client;

Client createClient(char* id, char* name, char* phone, char* email);
void destroyClient(Client cP);
void printc(Client cP);

#endif

client.c

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

struct client {
  char* id;
  char* name;
  char* phone;
  char* email;
};


// returns the pointer to the list; NULL if list not created
Client createClient(char* id, char* name, char* phone, char* email) {
  // allocate memory for a structure variable containing all
  // list components
  Client cptr = malloc(sizeof(struct client));
  // if allocation was succesfull
  if (cptr != NULL) {
     cptr->id = id;
     cptr->name = name;
     cptr->phone = phone;
     cptr->email = email;

  }
  return cptr;
}

void destroyClient(Client cptr) {
  free(cptr);
}

void printc(Client cptr){
  printf("%s %s %s %s\n", cptr->id, cptr->name, cptr->phone, cptr->email);
}

Here is clients.txt file

1212
Joseph Miller
206-555-1212
millers@comcast.net
1313
Beatrice Pizarro Ozuna
206-111-1111
bea@uw.edu
1314
Anne Simpson
425-777-8888
a.simpson@gmail.com
1100
Emily Price
206-111-5555
priceless@yahoo.com
1289
Sharon Henderson
206-555-1289
henderson21@comcast.net
1316
Sylvia Williamson
425-123-8888
sylvia@gmail.com
1101
Michael Murphy
425-111-5555
gemini@yahoo.com

The output of the first code is:

1101
 Michael Murphy
 425-111-5555
 gemini@yahoo.com

1101
 Michael Murphy
 425-111-5555
 gemini@yahoo.com

1101
 Michael Murphy
 425-111-5555
 gemini@yahoo.com

I don't understand why all array elements store the same element (the last one in the file). I want them to store respective elements. Please help.

You assigned the same pointer to every value. Copy your values when you create the instance. — Michael Fuller, May 30 '17 at 15:41

score 2 · Answer 1 · answered May 30 '17 at 15:51

In createClient, you need to duplicate the string you're passing in parameter, because they're actually pointers to your static char buffers you've declared in the first snippet. These get overwritten at every iteration in your while loop.

Try:

  if (cptr != NULL) {
     cptr->id = strdup(id);
     cptr->name = strdup(name);
     cptr->phone = strdup(phone);
     cptr->email = strdup(email);    
  }

And in destroyClient, make sure to free the memory that strdup has allocated implicitly:

void destroyClient(Client cptr) {
  free(cptr->id);
  free(cptr->name);
  free(cptr->phone);
  free(cptr->email);
  free(cptr);
}

Support Ukraine · Accepted Answer · 2017-05-30T16:20:30.913

Here

Client createClient(char* id, char* name, char* phone, char* email) {
  // allocate memory for a structure variable containing all
  // list components
  Client cptr = malloc(sizeof(struct client));
  // if allocation was succesfull
  if (cptr != NULL) {
     cptr->id = id;
     cptr->name = name;
     cptr->phone = phone;
     cptr->email = email;

  }
  return cptr;
}

you just assign the input pointer value, e.g. id, to the new client. Since all calls of createClient passes the same pointers from main, the result is that all created clients contains pointers to the same variables in main.

You'll need to allocate new memory for each and copy the data into the allocated memory. Something like:

Client createClient(char* id, char* name, char* phone, char* email) {
  // allocate memory for a structure variable containing all
  // list components
  Client cptr = malloc(sizeof(struct client));
  // if allocation was succesfull
  if (cptr != NULL) {
     cptr->id = malloc(256);  // Allocate memory
     strcpy(cptr->id, id);    // Copy data to the new memory
     // ... and so on
  }
  return cptr;
}

Notice: To keep the example simple, I skipped a check for malloc returning NULL. You should add that before the strcpy

Also notice, that you'll need to change destroyClient so that it frees all memory, e.g free(cptr->id).

An alternative approach:

As you are dealing with a fairly small amount of memory, it could be worth to consider to avoid the pointers in struct client and instead do:

struct client {
  char id[256];
  char name[256];
  char phone[256];
  char email[256];
};

Then you could do with a single malloc in createClient followed by strcpy (or even memcpy).

Every pointer in an array of pointers store the same value after reading in from a file in C

2 Answers2