Jenny Blunt Jenny Blunt - 3 months ago 15
C Question

Memory Leak Reading File to Linked List in C

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

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.