CODEWITHSUNDEEP
cdata-structuresstructcstring
Alexandre Strube
Alexandre Strube

Reputation: 142

How can I free memory from my structure containing an array of dynamically created strings?

I have a struct that must contain, among other things, a dynamically created string and an array of also dynamically-created strings. However, I am finding it difficult to free this struct afterwards, as it gives me runtime errors.

So the questions are:

So far I cannot even release the string. Why?

How to create a dynamic "array" of strings inside this struct?

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

typedef struct { 
  unsigned int num;
  unsigned int* sizes;
  unsigned int* coords;
  char* name;
} TOPOLOGY;

TOPOLOGY * constructor() {
  char * name="Hardware Topology";
  TOPOLOGY * top=calloc(1,sizeof(TOPOLOGY *));
  top->num=0;
  top->name=calloc(1,strlen(name));
  strcpy(top->name,name);
  return top;
}

void destructor(TOPOLOGY * top) {
  if(top->sizes!=NULL) { free(top->sizes); }
  if(top->coords!=NULL) { free(top->coords); }
  if(top->name!=NULL) { free(top->name); } exit(0);
  if(top!=NULL) { free(top); }
}

void add(TOPOLOGY * top, unsigned int size) {
  top->num++;
  size_t s=top->num*sizeof(unsigned int*);
  top->sizes=realloc(top->sizes,s);
  top->coords=realloc(top->coords,s);
  top->sizes[top->num-1]=size;
}

void coords(TOPOLOGY * top, unsigned int coords[]) {
  int i;
  for(i=0;i<top->num;i++) {
    top->coords[i]=coords[i];
  }
}

void get(TOPOLOGY * top) {
  int i;
  for(i=0; i<top->num;i++) {
    printf("Dim: %d: %d\n",i,top->sizes[i]);
  }
}

void getcoords(TOPOLOGY * top) {
  int i;
  for(i=0;i<top->num;i++) {
    printf("Coord %d = %d\n",i,top->coords[i]);
  }
}

void setname(TOPOLOGY * top, char * name) {
  int s=sizeof(name);
  top->name=realloc(top->name,s);
  strcpy(top->name,name);
}


int main() {
  unsigned int c[4]={3,2,0,1};
  TOPOLOGY * top=constructor();
  add(top,1025);
  add(top,512);
  add(top,10);
  add(top,24);
  coords(top,c);
  get(top);
  getcoords(top);
  destructor(top);
  return 0;
}

Upvotes: 1

Views: 7151

Answers (2)

cnicutar
cnicutar

Reputation: 182619

First up, the constructor is wrong. It will give you the size of a pointer on your machine, so pretty much everything you do with top from then on will be illegal.

TOPOLOGY * top=calloc(1,sizeof(TOPOLOGY *)); /* Wrong. */
                                        ^
top = calloc(1, sizeof(TOPOLOGY)); /* Better. */
top = calloc(1, sizeof(*top)); /* Even better. */

Second

Everywhere you calloc strings, you must use strlen(...) + 1.

top->name=calloc(1, strlen(name) + 1);
strcpy(top->name, name);

Or, slightly laterally, use strdup:

top->name = strdup(name); /* Does malloc and whatnot for you. */

Third

Don't check for NULL when freeing. It's perfectly legal to free(NULL).

void destructor(TOPOLOGY * top)
{
    free(top->sizes);
    free(top->coords);
    free(top->name);
    free(top);
}

Upvotes: 3

Kerrek SB
Kerrek SB

Reputation: 476990

Two things spring to mind:

  1. top->name=calloc(1,strlen(name)); Here you should allocate one more byte for the null terminator, or you'll create an unterminated string when you call strcpy. (Also, the local name in constructor() should be declared as const char *.)

  2. if(top->coords!=NULL) { free(top->coords); } You never initialize coords to zero. So if you just call destructor on a newly minted struct, you may very well be calling an invalid free().

Upvotes: 0

Related Questions