malloc() struct in a function, free() before ending program

Question

I am learning the basics of C, and now working with malloc(). Say I have a function that asks the user for input, populates a structure with said data, and that structure in saved in an array (all passed by reference from a main function).

I ran the program through Valgrind, I get the "still reachable" bytes (which should be ok right? not considered a leak), but any data saved I get lost blocks.

How would I go freeing that memory after the program has finished running? Also I have some (2) questions within the code just to clarify some things and would appreciate if someone could explain them to me.

Here is some code similar to what I am trying to do:

I have the following struct declared:

struct Person {
  char name[MAX_INPUT];
  int age;
};

I am writing a function that goes like this:

int function2(struct Person *list, int *index) {
  struct Person *prsn = malloc(sizeof(struct Person)); 
  // !Why do we sometimes cast the malloc or not? 
  // I sometimes get errors when I do, sometimes when I don't, 
  // while the surrounding code is pretty much the same.
  assert(prsn != NULL);

  // User input code goes here ... 

  // Now to save the Person created
  strcpy(prsn->name, nameInput);
  prsn->age = ageInput;
  list[(*index)++] = *prsn; 
  // !Why use the dereferencing *prsn here? 
  // why not directly prsn? Or is that saving the memory address and not very useful.

  return 0;
}

And this is my main function:

int main(int argc, char *argv[]) { 
  struct Person personList[MAX_SIZE];
  int index;

  function2(personList, &index);

  // Before closing, I want to free any mallocs I have done here. free()

  return 0;
}

Valgrind report:

LEAK SUMMARY:
==1766==    definitely lost: 44 bytes in 1 blocks
==1766==    indirectly lost: 0 bytes in 0 blocks
==1766==      possibly lost: 0 bytes in 0 blocks
==1766==    still reachable: 10,355 bytes in 34 blocks
==1766==         suppressed: 0 bytes in 0 blocks

Thank you in advance.

Edit: Fixed function2 parameters, return and other things. I apologize, was writing it quickly to illustrate my main question about freeing memory. Thanks for the corrections tips, but the real code is actually compiling correctly and everything.

Edit2: After adding a simple loop at the end of main like suggested to use free(), I get the following errors.

==2216== LEAK SUMMARY:
==2216==    definitely lost: 44 bytes in 1 blocks
==2216==    indirectly lost: 0 bytes in 0 blocks
==2216==      possibly lost: 0 bytes in 0 blocks
==2216==    still reachable: 10,355 bytes in 34 blocks
==2216==         suppressed: 0 bytes in 0 blocks
==2216== 
==2216== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)
==2216== 
==2216== 1 errors in context 1 of 2:
==2216== Invalid free() / delete / delete[] / realloc()
==2216==    at 0x563A: free (in /usr/local/Cellar/valgrind/3.8.1/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==2216==    by 0x10000194E: main (in ./test.out)
==2216==  Address 0x7fff5fbf9dd0 is on thread 1's stack
==2216== 
==2216== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

Answer 1

In object oriented programming languages, an array of objects is really just an array of pointers to the objects. So if a pointer takes 4 bytes, and an object takes 5, an array of 10 objects will actually be 4*10 bytes long (plus overhead).

MyClass[] my_variable = new MyClass[10]; // this will allocate 10*pointersize bytes, plus overhead, you still need to allocate more space for the objects themselves

In C, an array of structures is an array of structures. If a structure takes 8 bytes, an array of 10 of them takes 80 bytes. You don't need to allocate even more space. It's already there.

struct data_t my_variable[10]; // this will allocate 10*sizeof(data_t) bytes

So the easy answer is:

int function2(struct Person *list, int *index) {
  char* nameInput;
  // User input code goes here ...
  // IMPORTANT: the user input code must allocate the space for nameInput
  // if it doesn't, then a copy of the buffer must be made
  // instead of the direct assignment below

  // the memory for this instance of the person was already created when the
  // array was created, so just save the values

  list[*index].name = nameInput;
  list[*index].age = ageInput;
  *index += 1; 

  return 0;
}

When your code executes this line:

list[(*index)++] = *prsn; 

It is quite literally copying 8 bytes (on a 32 bit machine, sizeof(int) + sizeof(char*)), as I think you noticed something was up with your commment "// !Why use the dereferencing *prsn here?"

The address to the memory allocated with malloc isn't being copied in that line, but the contents of the memory are. That's why it is a memory leak, the address is lost when the function exits because it was never put into the array.

Answer 2

Code Analysis

Let's dissect this a bit at a time:

int function2(struct Person *list) {

As Iserni noted in his answer, this definition is inconsistent with your call to the function. I agree in general with his correction for the existing code (but I'm going to recommend modifying it shortly):

int function2(struct Person *list, int *index)


  struct Person *prsn = malloc(sizeof(struct Person)); 
  // !Why do we sometimes cast the malloc or not? 
  // I sometimes get errors when I do, sometimes when I don't, 
  // while the surrounding code is pretty much the same.

It depends on whether you're in C or C++; in C++, you must cast the return value from malloc() (if you use malloc() at all; you usually shouldn't in C++). In C, the cast is optional. There is a school of thought that omitting the cast reveals errors that inserting the cast can hide. I don't subscribe to that; I believe that malloc() should have been declared via <stdlib.h> and the compiler should be warning if there is no declaration in scope, and if there's a declaration in scope, the cast can't cover up sins. The other possible problem is that you assign a pointer to a non-pointer; that would be a mistake that the compiler should be complaining about too.

  assert(prsn != NULL);

This is not normally considered a sensible long-term method of handling memory allocation errors.

  // User input code goes here ... 

  // Now to save the Person created
  strcpy(prsn->name, nameInput);
  prsn->age = ageInput;
  list[(*index)++] = *prsn; 
  // !Why use the dereferencing *prsn here?

Because:

• list is a struct Person *.
• Therefore list[i] is a struct Person (albeit you spelled i as (*index)++).
• Therefore you must assign a struct Person to it.
• prsn is a struct Person *.
• Therefore *prsn is also a struct Person.
• Therefore the assignment is 'correct'.
• It also gives you the leak.
• You've overwritten the content of list[i] with the content of *prsn.
• You've not save the pointer to prsn anywhere.
• So you leak memory as you return from the function.

The surgery required to fix this is non-negligible:

int function2(struct Person **item)
...
*item = prsn;

and you have to revise the call; I'll come back to that when dissecting main().

  // why not directly prsn? Or is that saving the memory address and not very useful.
}

Your function is declared to return an int but you show no return. If you aren't going to return a value, declare the function as void, especially if you're going to ignore the return value as your code in main() does.

Your last comment is mostly covered by the discussion above; saving the memory address is crucial to stopping the leak so it is very useful.

And this is my main function:

int main(int argc, char *argv[]) { 
  struct Person personList[MAX_SIZE];
  int index;

Using an uninitialized variable is bad news. It is at best only accidentally zeroed. You can't afford to have random values in use as array indexes; initialize it explicitly. Also, we're going to need an array of pointers, not an array of structures:

  struct Person *personList[MAX_SIZE];
  int index = 0;

  ...other code...

  function2(personList, &index);

With the revised function:

  function2(&personList[index++]);

This is preferable; it means that function2() doesn't need to know about the array; you just pass it the address of the pointer to which the allocated memory pointer should be assigned. This reduces the coupling between your main() function and function2(), which makes the code simpler all around.

  // Before closing, I want to free any mallocs I have done here. free()

So you write:

  for (int i = 0; i < index; i++)
      free(personList[i]);

This releases all the memory allocated.

  return 0;
}

I like to see an explicit return at the end of main(), even though C99 says it isn't 100% necessary.

Make sure you are compiling with enough warnings enabled. If you're using GCC, then gcc -Wall should be the minimum level of compilation warning you run with (and you should have no warnings in your code when you do so). I run with more stringent warnings: gcc -std=c99 -Wall -Wextra -Wmissing-prototypes -Wold-style-definition -Wstrict-prototypes -Wshadow usually. You need to include -O3 (or some level of optimization) to get some warnings. The stuff about prototypes reflects paranoia about an old code-base I work with which still has K&R function definitions around.

---

Answering comments

First question while I go over your details and try things out: is there a memory impact between an array of structs and an array of pointers?

Yes, but it may not be what you're thinking of. If you use an array of structures, there's no need to allocate memory dynamic for the structures since the compiler's already allocated them for you. Since the purpose of the exercise is to use pointers and malloc(), it is better, therefore, to use pointers. In terms of space, there will be slightly more total memory used with the array of pointers (but there will be less memory leaked).

I am trying to change my code to use an array of pointers. But using function2(personList, &index); to call function2 now gives me the following warning: incompatible pointer types passing 'struct Person *[512]' to parameter of type 'struct Person *'. Is it OK if I write extra code in my main question to go into details? As a note, I'm trying to reference variables as much as possible, so as to not temporarily have the program copy data from function to function.

The compiler is correct if you've not made all the changes. Your code using two arguments copies more data between functions than my code using one argument.

Version 1

The following program uses the proposed single-argument function2() to reduce the coupling between the main() function and function2(), simplifying both.

This code compiles with no warnings under GCC 4.7.1 on Mac OS X 10.7.5 using the command line:

    gcc -O3 -g -std=c99 -Wall -Wextra -Wmissing-prototypes -Wstrict-prototypes \
        -Wold-style-definition mem.c -o mem

When run under valgrind, it comes up with no memory leaked.

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

enum { MAX_INPUT = 28 };
enum { MAX_SIZE  = 3  };

struct Person
{
  char name[MAX_INPUT];
  int age;
};

static void function2(struct Person **list)
{
    struct Person *prsn = malloc(sizeof(struct Person)); 
    assert(prsn != NULL);

    char *nameInput = "This is my name";
    int ageInput = 29;    // Again!

    strcpy(prsn->name, nameInput);
    prsn->age = ageInput;
    *list = prsn;
}

int main(void)
{
    struct Person *personList[MAX_SIZE];
    int index = 0;

    function2(&personList[index++]);
    function2(&personList[index++]);
    function2(&personList[index++]);

    for (int i = 0; i < index; i++)
        free(personList[i]);

    return 0;
}

Version 2

This keeps the two-argument version of function2() and has it do the counting that main() should be doing on its own. This program also compiles cleanly and runs cleanly under valgrind.

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

enum { MAX_INPUT = 28 };
enum { MAX_SIZE  = 3  };

struct Person
{
  char name[MAX_INPUT];
  int age;
};

static void function2(struct Person **list, int *index)
{
    struct Person *prsn = malloc(sizeof(struct Person)); 
    assert(prsn != NULL);

    char *nameInput = "This is my name";
    int ageInput = 29;    // Again!

    strcpy(prsn->name, nameInput);
    prsn->age = ageInput;
    list[(*index)++] = prsn;
}

int main(void)
{
    struct Person *personList[MAX_SIZE];
    int index = 0;

    function2(personList, &index);
    function2(personList, &index);
    function2(personList, &index);

    for (int i = 0; i < index; i++)
        free(personList[i]);

    return 0;
}

Answer 3

I assume that your function2 is really

int function2(struct Person *list, int *index)

You would save your malloc'ed pointer there with

list[(*index)++] = prsn;

Then in main you would free the list with

while(index)
    free(list[--index]);