Memory Leak Reading File to Linked List in C

Question

Learning how to use linked lists, I'm having trouble with a memory leak when reading a file into one.

My code looks as follows:

const struct dhcp_ops dhcp_exec;

struct dhcp_list {
  char mac[18];
  char ip[20];
  char name[255];
  struct dhcp_list *next;
}*conductor;

struct dhcp_ops {
  void (*clients)(struct dhcp_list **);
};

void get_clients(struct dhcp_list **buf)
{
  FILE *fp;
  char line[128];

  struct dhcp_list *ptr;
  ptr = (struct dhcp_list*)malloc(sizeof(struct dhcp_list));

  ptr->next = NULL;
  conductor = ptr;

  fp = fopen(DHCP_LEASES, "r");
  if(NULL == fp)
    return;

  char created[10];
  char mask[5];

  while(fgets(line, sizeof(line), fp) != NULL){
    ptr->next = malloc(sizeof(struct dhcp_list));
    if (ptr->next == NULL) break;

    sscanf(line, "%s %s %s %s %s\n",
        created,
        ptr->mac,
        ptr->ip,
        ptr->name,
        mask);

    ptr = ptr->next;
    ptr->next = NULL;
  }
  fclose(fp);

  *buf = conductor;
}

And the block that calls / formats into JSON, for demonstration.

void format_dhcp(json_object *jdhcp_array)
  {

    const struct dhcp_ops *dhcp;

    struct dhcp_list *clients = (struct dhcp_list*)malloc(sizeof(struct dhcp_list));

    dhcp = &dhcp_exec;
    dhcp->clients(&clients);

    while (clients->next != NULL) {
      printf("MAC ADDRESS: %s\n", clients->mac);

      json_object *jdhcp = json_object_new_object();

      json_object *jmac = json_object_new_string(tmp->mac);
      json_object_object_add(jdhcp, "mac", jmac);

      clients = clients->next;
    }

    struct dhcp_list *freeMe = clients;
    struct dhcp_list *holdMe = NULL;
    while(freeMe->next != NULL) {
      holdMe = freeMe->next;
      free(freeMe);
      freeMe = holdMe;
    }
    // free(clients);
 }

 const struct dhcp_ops dhcp_exec = {
   .clients       = get_clients,
 };

When I run this with valgrind, I get the following errors:

==2925== HEAP SUMMARY:
==2925==     in use at exit: 2,128 bytes in 7 blocks
==2925==   total heap usage: 1,756 allocs, 1,749 frees, 357,725 bytes allocated
==2925==
==2925== 304 bytes in 1 blocks are definitely lost in loss record 2 of 3
==2925==    at 0x4C27C0F: malloc (vg_replace_malloc.c:299)
==2925==    by 0x405176: format_dhcp (collector.c:479)
==2925==    by 0x4056DF: collect_data (collector.c:637)
==2925==    by 0x4057DB: collect_and_send_data (collector.c:669)
==2925==    by 0x405E35: monitor (monitor.c:165)
==2925==    by 0x40AA05: run_collector (boot.c:115)
==2925==    by 0x40AB10: boot (boot.c:152)
==2925==    by 0x409DE1: main (socketman.c:242)
==2925==
==2925== LEAK SUMMARY:
==2925==    definitely lost: 304 bytes in 1 blocks
==2925==    indirectly lost: 0 bytes in 0 blocks
==2925==      possibly lost: 0 bytes in 0 blocks
==2925==    still reachable: 1,824 bytes in 6 blocks
==2925==         suppressed: 0 bytes in 0 blocks
==2925== Reachable blocks (those to which a pointer was found) are not shown.
==2925== To see them, rerun with: --leak-check=full --show-leak-kinds=all

I tested by moving the 'free block' into the main function that reads the file. This runs perfectly, no leaks. Obviously, I don't get the output.

Could someone suggest where I'm going wrong / what I need to do to clear the memory correctly.

------------ EDIT SOLVED ----------------------

Following the comments and answers through, I implemented the changes. After, I'd got rid of the missing bytes. However, I had 1800+ still reachable.

==10669== LEAK SUMMARY:
==10669==    definitely lost: 0 bytes in 0 blocks
==10669==    indirectly lost: 0 bytes in 0 blocks
==10669==      possibly lost: 0 bytes in 0 blocks
==10669==    still reachable: 1,824 bytes in 6 blocks
==10669==         suppressed: 0 bytes in 0 blocks
==10669== Reachable blocks (those to which a pointer was found) are not shown.
==10669== To see them, rerun with: --leak-check=full --show-leak-kinds=all

Turns out, the clients were always after the first loop. So, I moved the freeMe struct above the first loop fixing the problem:

  void format_dhcp(json_object *jdhcp_array)
  {

    const struct dhcp_ops *dhcp;
    struct dhcp_list *clients = NULL;

    dhcp = &dhcp_exec;
    dhcp->clients(&clients);

    struct dhcp_list *freeMe = clients;

    while (clients->next != NULL) {
      printf("MAC ADDRESS: %s\n", clients->mac);

      json_object *jdhcp = json_object_new_object();

      json_object *jmac = json_object_new_string(clients->mac);
      json_object_object_add(jdhcp, "mac", jmac);

      json_object *jip = json_object_new_string(clients->ip);
      json_object_object_add(jdhcp, "ip", jip);

      json_object *jname = json_object_new_string(clients->name);
      json_object_object_add(jdhcp, "name", jname);

      json_object_array_add(jdhcp_array, jdhcp);

      clients = clients->next;
    }

    struct dhcp_list *holdMe = NULL;
    while(freeMe != NULL) {
      debug("Should be freed");
      holdMe = freeMe->next;
      free(freeMe);
      freeMe = holdMe;
    }
}

Answer 1

With the condition freeMe->next != NULL, the last element won't be freed.

18 + 20 + 255 = 293, so the "lost" 304 bytes should be one struct dhcp_list. (The left 11 bytes should be one pointer + padding)

Try using freeMe != NULL instead.

Also note that they say you shouldn't cast the result of malloc() in C.

---

Here is another memory leak:

You allocated some memory in the line

struct dhcp_list *clients = (struct dhcp_list*)malloc(sizeof(struct dhcp_list));

but you throwed the address away without using it at all in dhcp->clients, which is get_clients. Stop allocating such a wasteful memory and just do variable declaration like

struct dhcp_list *clients;

or to make it safer for in case dhcp->clients didn't work well, initialize it to NULL and check if its value is still NULL after calling the function.

Answer 2

The problem is here in format_dhcp:

struct dhcp_list *clients = (struct dhcp_list*)malloc(sizeof(struct dhcp_list));
                  ^^^^^^^ here you allocate memory for clients
dhcp = &dhcp_exec;
dhcp->clients(&clients);
      ^^^^^^ here you call get_clients

In get_clientsyou do:

ptr = (struct dhcp_list*)malloc(sizeof(struct dhcp_list));

ptr->next = NULL;
conductor = ptr;
....
*buf = conductor;
^^^^^
Now the original clients is lost and you have a leak

You never use the memory allocated to clients inside format_dhcp and you overwrite the pointer in get_clients which causes a leak.

So the conclusion is: Don't do the malloc in format_dhcp - you don't need it.