This is a little test to help demonstrate and review the ouput of valgrind's memcheck. Can someone help me figure out how to remove AND free a node from the middle of the list? If I comment out the free(cur) and free(cur->lock) from the remove node section, then memcheck tells me I have a memory leak, but if I keep them there then I am doing an invalid read at the top of the loop. Is there a way out of this conundrum?
TEST(UtilityGeneralUnittest, valgrindTests)
{
//declare a node type
typedef struct node{
int size;
int value;
struct node *next;
pthread_mutex_t *lock;
}node_t;
//make the head
node_t *head;
head = (node_t*)malloc(1 * sizeof(node_t));
head->size = 0;
head->next = NULL;
head->lock = (pthread_mutex_t*)malloc(1 * sizeof(pthread_mutex_t));
pthread_mutex_init(head->lock, NULL);
//create array for storing values
int array[10];
//build a list with random numbers
for (int i = 0; i < 10; i++)
{
node_t *newNode;
newNode = (node_t*)malloc(1 * sizeof(node_t));
newNode->value = rand() % 100 + 1;
newNode->next = NULL;
newNode->lock = (pthread_mutex_t*) malloc(1 * sizeof(pthread_mutex_t));
pthread_mutex_init(newNode->lock, NULL);
array[i] = newNode->value;
if (head->next == NULL)
{
head->next = newNode;
head->size++;
}
else
{
node_t *tmp = head->next;
head->next = newNode;
newNode->next = tmp;
head->size++;
}
}
// assert the list added nodes
ASSERT_EQ(10, head->size);
//sanity check; print the list
node_t *printer = head;
while(printer->next != NULL)
{
printer = printer->next;
std::cout << "value: ";
std::cout << printer->value << ", ";
}
std::cout << "\n";
// the meat and potatoes: deleting with locks.
int removeMe = array[rand() % 10];
bool verifyDel = true;
int checkVal = removeMe;
node_t *prev;
node_t *cur;
prev = head;
pthread_mutex_lock(prev->lock);
while((cur = prev->next) != NULL) //******** this is the problem
{
pthread_mutex_lock(cur->lock);
if(cur->value == removeMe)
{
prev->next = cur->next;
pthread_mutex_unlock(cur->lock);
pthread_mutex_unlock(prev->lock);
cur->next = NULL;
head->size--;
free(cur->lock); ///******** this is the problem
free(cur); ///****** this is the problem
}
pthread_mutex_unlock(prev->lock);
prev = cur;
}
//pthread_mutex_unlock(prev->lock);
//verify node has been removed
printer = head;
while(printer->next != NULL)
{
printer = printer->next;
if(printer->value == checkVal)
{
verifyDel = false;
}
std::cout << "value: ";
std::cout << printer->value << ", ";
}
std::cout << "\n";
ASSERT_TRUE(verifyDel);
//clean up: delete the list
while((printer = head) != NULL)
{
head = head->next;
free(printer->lock);
free(printer);
std::cout << "free!!!" << std::endl;
}
}
Looking at your loop (simplified):
while ((cur = prev->next) != NULL) //* problem
{
if (cur->value == removeMe)
{
prev->next = cur->next;
cur->next = NULL;
free(cur); //* problem
}
prev = cur;
}
The problem is with the assignment prev = cur
, but only when the if
block is evaluated. That has freed cur
, so next time around the loop, cur=prev->next
references deleted storage.
You can fix that by inserting else
to assign prev
only when cur
wasn't removed:
while ((cur = prev->next)) {
if (cur->value == removeMe) {
prev->next = cur->next;
cur->next = NULL;
free(cur);
} else {
prev = cur;
}
}