uninitialised memory in my singly linked list implementation
I tried to implement a singly linked linked List in c. I wanted to be able to use multiple instances of the list and I wanted to create the list in the main function. That is why i chose to implement it in the way I did.
The code works perfectly fine but i am concerned because of the output valgrind creates. Also I tried to use the code in a project on an embedded system and strange errors happen.
The valgrind output is:
starting...
==3570== Conditional jump or move depends on uninitialised value(s)
==3570== at 0x100000E8E: push_cfront (in ./map_test)
==3570== by 0x100000D4F: main (in ./map_test)
==3570== Uninitialised value was created by a heap allocation
==3570== at 0x100008EBB: malloc (in /usr/local/Cellar/valgrind/3.11.0/lib/valgrind/vgpreload_memcheck-amd64-darwin.so)
==3570== by 0x100000E80: push_cfront (in ./map_test)
==3570== by 0x100000D4F: main (in ./map_test)
==3570==
...finished
Also it tells me that i am loosing one block. Where do i make a mistake freeing it
==3570== LEAK SUMMARY:
==3570== definitely lost: 16 bytes in 1 blocks
==3570== indirectly lost: 0 bytes in 0 blocks
==3570== possibly lost: 2,064 bytes in 1 blocks
==3570== still reachable: 0 bytes in 0 blocks
==3570== suppressed: 24,525 bytes in 186 blocks
Please give me a hint on where i went wrong.
test.c:
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include "command_list.h"
int main() {
printf("starting...n");
Clist * command_list = malloc(sizeof(Clist));
if (command_list == NULL) printf("Malloc Failedn");
command_list->head = NULL;
//push_cback(command_list, 0);
push_cfront(command_list,1);
free_clist(command_list);
free(command_list);
printf("n...finishedn");
return 0;
}
command_list.h:
#ifndef __COMMAND_LIST_H
#define __COMMAND_LIST_H
typedef struct cnode {
uint8_t command;
struct cnode * next;
} Cnode;
typedef struct clist {
Cnode * head;
} Clist;
void push_cback(Clist * list, uint8_t command);
void push_cfront(Clist * list, uint8_t command);
void free_clist(Clist * list);
#endif
command_list.c
void push_cfront(Clist * list, uint8_t command){
Cnode * new_node;
new_node = malloc(sizeof(Cnode));
if (new_node->next == NULL) {
return;
}
new_node->command = command;
new_node->next = list->head;
list->head = new_node;
}
void free_clist(Clist * list){
if (list->head == NULL){
return; //already empty
}
Cnode * current = list->head;
while (current->next != NULL){
Cnode* temp = current->next;
free(current);
current = temp;
}
free(current);
list->head = NULL;
}
You have a few problems. You are checking new_node->next (uninitialized data in malloc'd memory) instead of new_node (the return value of malloc). On my computer at least this also causes memory not to be freed because by chance new_node->next is null so you return without freeing new_node . Also, if you want to support pushing to the back of a linked list, you should consider a circularly linked list because it allows that operation without having to traverse the whole thing.
Finally, a few tips: it's good that you are using valgrind, but it will be more helpful if you compile with -g to enable debugging symbols so valgrind will tell you line numbers. Also, when I make linked lists, I like to use a dummy head node for some operations to avoid a special case for empty or singleton lists. For inserting into a sorted linked list, that technique looks like:
int sorted_insert(Clist *list, char new_command){
Cnode _head = {NULL, list->head}, *head = &_head, *prev = head, *tmp;//head is an auto dummy node obviating null checks.
int ord = -1;//If there are no existing nodes, newObj would be less than all objects.
while(prev->next && (ord = (int)newObj - prev->next->command)) > 0){//Iterate by prev->next not curr to use only one pointer.
prev = prev->next;//Looping while there is a next node and its data compares less than new_command.
}
if((!ord) || !(tmp = malloc(sizeof(Cnode))){//newObj is already in the list or allocation failed.
return 0;
}
*tmp = (Cnode){.next=prev->next, .command=new_command};
prev->next = tmp;
list->head = head->next;//un- add head which is then deallocated by stack frame cleanup.
return 1;
}
The new_node->next is uninitialized when you check it's value. You don't need that.
If you look for malloc failures, set a return code for the function and check it upon invocation.
Until that, the if branch in push_cfront is not needed.
... or you wanted to check list->head instead?
Also there's a problem with this piece of code in push_cfront
new_node = malloc(sizeof(Cnode));
if (new_node->next == NULL) {
return;
}
It's undefined behaviour since new_node memory is not initialised. You probably wanted to check if (new_node == NULL) to see if memory was actually allocated.
上一篇: 我的Valgrind装置是否损坏？
下一篇: 未初始化的内存在我的单链表实现中