0

I am working on a C homework, and I am finding it pretty confusing to detect what memory was not de-allocated.

I been trying to place free() function in different locations on my code just to check whether would it work or not but none of the trys has paid off yet.

this is the struct and the functions used for it; all the functions below are inside one file called state.c - stateAdd calls stateNodeCreate -

typedef struct node_t {
    int id;
    const char* name;
    const char* song;
    Set votes;
    Set grades;
    int top_grade;
}*StateNode;

void stateNodeDestroy(StateNode node) {
    if(node == NULL) {
        return;
    }
    if(node->votes) {
        setDestroy(node->votes);
    }
    if(node->grades) {
        setDestroy(node->grades);
    }
    free(node);
}


static StateNode stateNodeCreate(int id,const char* name,const char* song) {
    StateNode state = (StateNode)malloc(sizeof(*state));
    if(state == NULL) {
        return NULL;
    }
    state->id = id;
    state->name = name;
    state->song = song;
    state->top_grade = -1;
    Set votes_set = setCreate(copyVoteElement,
                              freeVoteElement, compareVotes);
    if(votes_set == NULL) {
        free(state);
        return NULL;
    }
    state->votes = votes_set;
    Set grades_set = setCreate(copyGradeElement,
                               freeGradeElement, compareGrades);
    if(grades_set == NULL) {
        stateNodeDestroy(state);
        setDestroy(grades_set);
        return NULL;
    }
    state->grades = grades_set;
    return state;
}

stateResult StateAdd(Set states, int stateId, const char* stateName, const char* songName) {
    if (states == NULL) {
        return STATE_NULL_ARGUMENT;
    }
    if(stateId < 0 ) {
        return STATE_INVALID_ID;
    }
    if(!IsValidName(stateName) || !IsValidName(songName)) {
        return STATE_INVALID_NAME;
    }
    StateNode state_exist = findState(states,stateId);
    if(state_exist != NULL) {
        return STATE_STATE_ALREADY_EXIST;
    }
    free(state_exist);
    StateNode new_state = stateNodeCreate(stateId,stateName,songName);
    if(new_state == NULL) {
        return STATE_OUT_OF_MEMORY;
    }

    if(setAdd(states,new_state) == SET_OUT_OF_MEMORY) {
        stateNodeDestroy(new_state);
        setDestroy(states);
        return STATE_OUT_OF_MEMORY;
    }
    return STATE_SUCCESS;
}

setDestroys destroys an already made set using setCreate and setCreate has function pointers as parameters and it uses these functions to copy/free data from the sets (an ADT)

Below are functions from another file: eurovision.c : eurovisionAddState calls stateAdd that exists in state.c file -

typedef struct eurovision_t {
    Set states;
    Set judges;
} *Eurovision;

EurovisionResult eurovisionAddState(Eurovision eurovision, int stateId, const char *stateName, const char *songName) {
    stateResult result = StateAdd(eurovision->states, stateId, stateName, songName);
    if (result == STATE_NULL_ARGUMENT) {
        return EUROVISION_NULL_ARGUMENT;
    }
    if (result == STATE_OUT_OF_MEMORY) {
        //setDestroy(eurovision->judges);
        eurovisionDestroy(eurovision);
        return EUROVISION_OUT_OF_MEMORY;
    }
    if (result == STATE_INVALID_ID) {
        return EUROVISION_INVALID_ID;
    }
    if (result == STATE_STATE_ALREADY_EXIST) {
        return EUROVISION_STATE_ALREADY_EXIST;
    }
    if (result == STATE_INVALID_NAME) {
        return EUROVISION_INVALID_NAME;
    }
    return  EUROVISION_SUCCESS;

}

void eurovisionDestroy(Eurovision eurovision) {
    if (eurovision == NULL) {
        return;
    }
    if(eurovision->judges != NULL) {
        setDestroy(eurovision->judges);
    }
    if(eurovision->states != NULL) {
        setDestroy(eurovision->states);
    }
    free(eurovision);
}

eurovisionDestroy clears the Sets inside of the parameter eurovision and frees the eurovision element. And the last part is this function that exist in test.c: The function testAddState calls the function eurovisionAddState that exists in eurovision.c file -

bool testAddState() {
  Eurovision eurovision = setupEurovision();
  CHECK(eurovisionAddState(eurovision, 0, "israel", "home"), EUROVISION_SUCCESS);
  //CHECK(eurovisionAddState(eurovision, 1, "malta", "chameleon"), EUROVISION_SUCCESS);
  //CHECK(eurovisionAddState(eurovision, 0, "croatia", "the dream"), EUROVISION_STATE_ALREADY_EXIST);
  //CHECK(eurovisionAddState(eurovision, 0, "israel", "home"), EUROVISION_STATE_ALREADY_EXIST);
  //CHECK(eurovisionAddState(eurovision, -1, "croatia", "the dream"), EUROVISION_INVALID_ID);
  eurovisionDestroy(eurovision);
  return true;
}

after all that, when using valgrind to detect memory leaks, It still shows that in this piece of code, there are memory leaks. Valgrind says the following:

48 bytes in 1 blocks are definitely lost in loss record 1 of 1
==10451==    at 0x4C29BC3: malloc (vg_replace_malloc.c:299)
==10451==    by 0x402A71: stateNodeCreate (state.c:124)
==10451==    by 0x402DA9: StateAdd (state.c:236)
==10451==    by 0x40082D: eurovisionAddState (eurovision.c:49)
==10451==    by 0x4015E9: testAddState (eurovisionTests.c:169)
==10451==    by 0x4027CE: main (eurovisionTestsMain.c:15)

This is not the whole code, I am very positive that the rest works just fine to not cause any memory leaks to this piece of code. Any ideas?

trincot
  • 317,000
  • 35
  • 244
  • 286
  • *"I am very positive that the rest works just fine to not cause any memory leaks to this piece of code"* I wouldn't be so sure of that. My money is on `setAdd` or `setDestroy`. – dbush May 03 '19 at 18:07
  • Regarding `state->name = name`, what happens when the memory pointed to by `name` changes content? Or if the memory itself becomes invalid (deallocated for example)? – Some programmer dude May 03 '19 at 18:11
  • And [in C you should not cast the result of `malloc`](https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc). – Some programmer dude May 03 '19 at 18:11
  • Finally about your problem: Time to [learn how to debug your programs](https://ericlippert.com/2014/03/05/how-to-debug-small-programs/). Rubber duck debugging, printf-debugging, and stepping throuhg the code in an actual debugger, you might need to do all of those to find out when and where the leak is. – Some programmer dude May 03 '19 at 18:15
  • And do the `set*` functions work as expected? You have unit-tests for them as well? – Some programmer dude May 03 '19 at 18:17
  • Yeah I am sure the Set library works just fine since It was made by the teaching team. @dbush – Ameer Shehadey May 03 '19 at 18:21
  • @Someprogrammerdude I been debugging the whole day, do you have any tips to debug using CLion IDE? It is kinda hard to track a memory leak. – Ameer Shehadey May 03 '19 at 18:22
  • I was not expecting to get any help when I made the topic, I was just hoping someone would find something I can't see, I know the question size is kinda large.. – Ameer Shehadey May 03 '19 at 18:24
  • @AmeerShehadey Just because the teaching team made the set library doesn't mean it doesn't have a leak. I don't see a leak in this code. Add `printf` calls in `setAdd` and `setDestroy` in strategic places to see what it's doing. – dbush May 03 '19 at 18:25
  • Everywhere you allocate memory and everywhere you do a `free`, call `printf` printing the pointer allocated/free'd (and from where it's done). Then you can try to match the allocations and the freeing. If you notice that there's a `free` missing, then you go to that function and add more `printf` calls everywhere it's called. And add `printf` calls all the way up to `eurovisionDestroy`. Then you can easily see when the call-chain stops, and add do some more debugging concentrated on those parts only. – Some programmer dude May 03 '19 at 18:30
  • @Someprogrammerdude thanks fam, seems a good idea. I will be trying it asap. – Ameer Shehadey May 03 '19 at 19:17
  • @AmeerShehadey I used to find bugs in teacher provided code libraries all of the time. Don't trust them to be correct. – Zan Lynx May 03 '19 at 19:38
  • @ZanLynx could be, but I don't have any access to their files. All I could do is #include set.h and the real functions are found in an encrypted file called libmtm.a, so it is not .c and I can't really see how they made their functions. – Ameer Shehadey May 03 '19 at 21:11

0 Answers0