Freeing the temp node when adding to a linkedlist

Question

I have a function called addMod that, when called, adds a node to a certain index of an array of Module struct LinkedLists called modules contained within a System struct. A Module struct has a string field, two int fields, and a pointer to the next Module, the first three fields being initialized according to arguments provided in addMod. addMod roughly looks like this:

int addMod(System *system, const char *text, int num1, int num2, int index) {
    Module *temp = malloc(sizeof(Module));
    Module *current;
    temp->next = NULL;

    if ([any of the constructors are invalid]) return 0;

    temp->text = malloc(strlen(text)+1);
    strcpy(temp->text, text);
    temp->num1 = num1; temp->num2 = num2;

    if (!system->modules[index]) {
        system->modules[index] = temp; //If there are no modules in the LinkedList at the given index, makes the head = temp.
    }
    else {
        if (system->compare(temp, system->modules[index]) <= 0) { //compare is a func pointer field of system that compares two Modules to see in what order they should be. Here, we check if temp should become the head of modules[index].
            temp->next = system->modules[index]; //Assigns the current head as the module following temp.
            system->modules[index] = temp; //Makes temp the current head.
        }
        else {
            current = system->modules[index];
            while (current->next && system->compare(temp, current->next) > 0) { //While current isn't the last node in the LinkedList and temp comes after the node after current
                current = current->next;
             }
            temp->next = current->next; //Adds temp in between current and current->next.
            current->next = temp;
        }
    }
    return 1;
}

All of the above works as expected, except when printing the contents of system, the console indicates there's a memory leak that I'm assuming is because I fail to properly free temp based on what valgrind tells me. My problem is not knowing where to free it- it seems anywhere I put it causes a segfault after printing the contents. From my understanding, I have to make sure that no other variables are depending upon the value being held by temp, but I can't seem to find a way to do that considering every possible ending of my if statement leads to assigning temp to a node within modules. Putting free(temp) between the logic and return 1 also yields a segfault, I'm assuming because I often malloc temp again when calling addMod multiple times in succession.

In summary, to add a new node to a LinkedList that may or may not be populated, in which this new node may be inserted in any arbitrary position in the LinkedList, I have to allocate memory to a temporary node so that I can insert it later. Where do I free this allocated memory once I have successfully inserted the node?

Answer 1

Assuming your management of a System instance is sound (a big assumption, since I cannot see that code), you have giant hole in the memory allocation of temp with a subsequent hard return 0 in the condition where the "constructor" check fails. More to the point:

Module *temp = malloc(sizeof(Module)); // memory allocated here...
Module *current;
temp->next = NULL;

if ([any of the constructors are invalid]) 
    return 0; // and leaked here.

It may be as simple as swapping the check around. Obviously other code that is supposed to free the dynamic allocations should be considered and evaluated as well.

---

A Simpler Approach

The node addition code is complicated and it need not be. In the end all you should really care about is finding the place where your new node resides.

1. If the slot in the table is empty, its the first node in that list.
2. IF the slot in the table is NOT empty, find the sorted location and insert it there.

Both of those can be accomplished with a single while-loop by using a pointer-to-pointer, where said entity hold the address of the pointer that will hold the new node in either of the cases above, and as a bonus, surgical insertion is literally two assignments.

It's done like this. Note that most of this code is just making the Module object safely. The actual insertion is only a single while-loop and some pointer assignments. It assumes the table in System initially contains NULL entries:

int addMod(System *system, const char *text, int num1, int num2, int index)
{
    // allocate new node here
    Module *temp = malloc(sizeof *temp);
    if (temp == NULL)
    {
        perror("Failed to allocate new Module");
        return 0;
    }


    size_t len = strlen(text);
    temp->text = malloc(len + 1);
    if (temp->text == NULL)
    {
        perror("Failed to allocate module name");
        free(temp);
        return 0;
    }

    // finish copying member data
    memcpy(temp->text, text, len);
    temp->text[len] = 0;
    temp->num1 = num1;
    temp->num2 = num2;

    // now find where it belongs, and set next appropriately    
    Module **pp = system->modules + index;
    while (*pp && system->compare(temp, *pp) <= 0)
        pp = &(*pp)->next;

    temp->next = *pp;
    *pp = temp;
    return 1;
}

Understand this is from deriving what I think your System type looks like, as it was never presented:

typedef struct System
{
    Module *modules[MAX_MODULES];
    int (*compare)(const Module* lhs, const Module *rhs);

} System;

I'm fairly confident it is similar to this. Of course, you'll have to adapt if it isn't. I suggest you review this and step through it in a debugger. There is no substitute for watching it live.

Best of luck.