C memory management beginner question

Question

OK, I have this piece of code:

typedef struct faux_crit
{
  char dna[DNALEN+1]; //#define'd to 16
  int x, y;
  int age;
  int p;
  int dir;
} crit;

crit *makeguy(int x, int y)
{
  crit *guy;
  guy = (crit *) malloc(sizeof(crit));
  strcpy(guy->dna, makedna());
  guy->x = x;
  guy->y = y;
  guy->age = guy->p = guy->dir = 0;
  return guy;
}

char *makedna()
{
  char *dna;
  int i;
  dna = (char *) malloc(sizeof(char) * DNALEN+1);
  for(i = 0; i < DNALEN; i++)
    dna[i] = randchar();
  return dna;
}

int main()
{
  int i;
  crit *newguy;
  srand((unsigned) time(0));

  newguy = makeguy(0, 0);
  /*[..]
   just printing things here
   */
  free(newguy);

  return 0;
}

I'd just want to know what did I wrong with managing memory, because valgrind reports a memory error. I presume it's the dna var in makedna, but when should I free it? I don't have access to it after leaving the function, and I need to return it, so I can't free it before that.

EDIT: Okay, thank you all.

Answer 1

You should store the pointer of makedna() before passing it to strcpy so you will be able to free it once you're done.

char* dna = makedna();
strcpy(guy->dna, dna);
free(dna);

Answer 2

Line 3 : dna[DNALEN+1] // statically declared array containing [DNALEN+1] elements - memory is already assigned Line 25: no need for the malloc as the array already exists

Answer 3

You guessed right. The problem is related to the makedna function. However, there is nothing wrong with that function. It is more about how you use it.

It is perfectly OK for makedna to return a pointer to allocated memory. However, it is up to the calling function to release that memory after it is done with it.

Thus I recommend you change:

crit *guy;
guy = (crit *) malloc(sizeof(crit));
strcpy(guy->dna, makedna());

to:

crit *guy;
guy = (crit *) malloc(sizeof(crit));
char * dna = makedna();
strcpy(guy->dna, dna);
// Now that we copied the content of the dna string
// we can free it.
free(dna);

EDIT: As Chris Jester-Young points out, there was a small problem with your makedna function as well. You need to null-terminate the value you return from that function so that strcpy can work correctly. Add this in your makedna function before you return the result:

dna[DNALEN] = 0;

Answer 4

You should do this:

char *tempdna = makedna();
strcpy(guy->dna, tempdna);
free(tempdna);

But for the strcpy to work, your makedna function needs to zero-terminate the string. At the end, just before the return, have:

dna[DNALEN] = 0;

Answer 5

fbereton is right - you didn't need a malloc for DNA.

However, had you been storing malloc'd memory inside your structure, the key point is that you can't just free the structure and expect the other memory allocations to be freed. You'd want to free all the allocations inside the structure before freeing the structure itself.

Answer 6

You should change makedna() to take a parameter:

void makedna(char* dna)
{
  int i;
  for(i = 0; i < DNALEN; i++)
  {
    dna[i] = randchar();
  }
}

note the extra braces for clarity

and line 14 becomes:

makedna(guy->dna);

This avoids at least one set of messing about with malloc and free.
Edit:
Also this solution avoids any null-termination problems with strcpy.

Answer 7

The easiest workaround is to change makeguy() like this:

char* dna = makedna();
strcpy(guy->dna, dna);
free(dna);

But this is not a good solution as you are allocating memory at one location and freeing it in other. It is better to do the malloc and free at the same place. So I recommend to change makedna() to:

void* makedna(char* dna, int dna_len)
{
  int i;
  for(i = 0; i < dna_len; i++)
    dna[i] = randchar();
}

You can call makedna() like this:

char* dna = (char*)malloc(DNALEN+1);
makedna(dna, DNALEN);
dna[DNALEN] = 0;
strcpy(guy->dna, dna);
free(dna);

Now makedna() only does what it is expected to do: make a dna sequence. Memory management should be taken care of by the caller. Moreover, this solution gives the flexibility of using a static char array, if that is required at a different call site.