Trouble freeing all my memory

Question

I am quite new to C and am having a hard to grasping full understanding of pointers
The current task at hand is clearing every single array from the struct Commands (the struct is shown on the bottom).
Hopefully someone can identify something horribly wrong like freeing something multiple times. I know my code might look horrid but thanks for taking the time to look at it.
My current attempt at clearing everything:

void clear_commands(Commands *commands) {
  Command *compile, *test, *temp;
  if (commands != NULL) {
    compile = commands->compile;
    while (compile != NULL) {
      temp = compile;
      compile = compile->next;
      free(temp);
    }

    test = commands->test;
    while (test != NULL) {
      temp = test;
      test = test->next;
      free(temp);
    }

    free(commands);
    commands = NULL;
  }
}

These are the typedefs:

typedef struct Command{
  char *command;
  struct Command *next;
} Command;

typedef struct {
  Command *test;
  Command *compile;
} Commands;

ADDED: read_commands

Commands read_commands(const char *compile_cmds, const char *test_cmds) {
  FILE *f;
  Commands *commands = malloc(sizeof(Commands));
  Command *compile, *test;
  char temp[257];

  if (compile_cmds == NULL || test_cmds == NULL)
    exit(0);

  compile = malloc(sizeof(Command));
  test = malloc(sizeof(Command));
  commands->compile = compile;
  commands->test = test;

  f = fopen(compile_cmds,"r");
  if (f != NULL)
    while(!feof(f)) {
      if(fgets(temp, 257, f) != NULL) {
        compile = malloc(sizeof(Command));
        compile->command = malloc(strlen(temp) + 1);
        strcpy(compile->command, temp);
        compile->next = malloc(sizeof(Command));
        compile = compile->next;
      }
    }
  fclose(f);
  ...
  return *commands;
}

Answer 1

free(commands->compile); and free(commands->test); are not necessary, you are freeing them in the first loop

---

clear_commands is good, the problem is in read_commands

          commands->compile = NULL; /* use malloc here */
          commands->test = NULL; /* and here */
          compile = commands->compile; /* otherwise you are giving NULL pointer variable to compile */
          test = commands->test; /* Same here */

          f = fopen(compile_cmds,"r");
          if (f != NULL)
            while(!feof(f)) {
                if(fgets(temp, 257, f) != NULL) {
                    compile = malloc(sizeof(Command)); /* this is not necessary */
                    compile->command = malloc(strlen(temp) + 1);
                    strcpy(compile->command, temp);
                    compile->next = NULL; /* use malloc here */
                    compile = compile->next; /* if not, compile is NULL again */
                }
           }
           fclose(f);

            /*after this add:*/

if (compile == commands->compile) {
    commands->compile = NULL;
}
free(compile);
compile = NULL;

same things with the test_cmds

Answer 2

There are a few things that I see with your read_commands, which are not exactly correct!

Your commands->compile and commands->test are always NULL as you set them at the beginning of the code! Creating a new variable compile = commands->compile and then running all the code using compile does NOT mean that your original commands->compile will also be modified. So, after your first fclose(f); your compiles->next is still NULL, because you just did not change it, you changed another variable compile.

And this is mistake is there even when you are probably changing commands->test, compile->next, or test->next.

You need to understand that when you copy the value of a first pointer in second pointer variable, and you change the value of the variable that is pointed by the second, the value when accessed using first also shows the change. Example ::

int a = 10, *b, *c;
b = &a;
c = a;
*c = 30; // a gets changed

It does not mean if you change the second itself, that will also change the value pointed by first. Example ::

int a = 10, *b, *c, *d, e = 30;
b = &a;
d = &e;
c = b;
c = d; //This does not change a

This is something you are trying to do everywhere in your read_commands code. I think you can get an idea from this and understand your mistake!

Edit ::

A part of the corrected code ::

  Command *prev = NULL;
  f = fopen(compile_cmds,"r");
  if (f != NULL)
    while(!feof(f)) {
      if(fgets(temp, 257, f) != NULL) {
        Command *var = malloc(sizeof(Command));
        var->command = malloc(strlen(temp) + 1);
        var->next = NULL;
        strcpy(var->command, temp);
        if(prev == NULL) {
            compile = var;
            prev = compile;
        } else {
            prev->next = var;
            prev = var;
        }
      }
    }
    commands->compile = compile;

Answer 3

A recursive solution is the simplest and easiest to comprehend solution in this case; so long as your lists are not so long that the stack overflows due to the recursion depth:

void clear_command_list( Command* cmd_list )
{
    // If this node exists...
    if( cmd_list != NULL )
    {
        // Clear the tail list
        clear_command_list( cmd_list->next ) ;

        // Free command string in this node
        free( cmd_list.command ) ;
        cmd_list.command = NULL ;

        // Free Command node
        free( cmd_list ) ;
        cmd_list = NULL ;
    }
} 

void clear_commands(Commands *commands)
{
    // Delete test list
    clear_command_list( commands->test ) ;

    // Delete compile list
    clear_command_list( commands->compile ) ;

    // Delete the Commands header
    free( commands ) ;
    commands = NULL ;
}

Note that setting the pointers to NULL following free() protects against heap corruption should the pointer be erroneously free'd again.

Each list can be thought of as a node followed by a smaller list. The clear_command_list() function calls itself with the "smaller" list, then deletes the head data. The recursion becomes deeper and deeper until it finds the terminating NULL node, the the recursion unwinds deleting each node from the end-backwards.

Even if you do it iteratively, a separate clear_command_list() function that can delete both compile and test lists makes sense rather that repeating the same code with different variables - that is maintenance heavy and error prone. Get it right once and re-use it. The structure is the same as above (and the clear_commands() function identical, only clear_command_list() needs to change:

void clear_command_list( Command* cmd_list )
{
    Command* next = cmd_list ;

    // For each lode in list
    while( next != NULL )
    {
        Command* node = next ;
        next = node->next ;

        // Free command string in this node
        free( node->command ) ;
        node->command = NULL ;

        // Free Command node
        free( node ) ;
        node = NULL ;
    }
} 

Answer 4

Commands read_commands(const char *compile_cmds, const char *test_cmds) {
  FILE *f;
  Commands *commands = malloc(sizeof(Commands));
  Command *compile, *test;
  char temp[257];

  if (compile_cmds == NULL || test_cmds == NULL)
    exit(0);

  compile = malloc(sizeof(Command));
  test = malloc(sizeof(Command));
  commands->compile = compile;
  commands->test = test;

  f = fopen(compile_cmds,"r");
  if (f != NULL)
    while(!feof(f)) {
      if(fgets(temp, 257, f) != NULL) {
        compile->command = malloc(strlen(temp) + 1);
        strcpy(compile->command, temp);
        compile->next = malloc(sizeof(Command));
        compile = compile->next;
      }
    }
  fclose(f);
  ...
  return *commands;
}