1

I'm having trouble with this small part of the code that generates errors in valgrind. When I comment the code and run valgrind, I dont get any memory leak or error so this loop should be the cause:

///Print the top users
    const char* str;
    for (int i = 0; i < count; i++) {
        if (FinalArray[i].Score == -1) {
            break;
        }

        int id = UserGetID(user);
        char* name = UserGetName(user);
        int finalID = UserGetID(FinalArray[i].user);
        char* finalName = UserGetName(FinalArray[i].user);

        assert(finalName!= NULL && name !=NULL);
        str = mtmSuggestFriends(id, name, finalID, finalName);

        if (str == NULL) {
            return MAIN_ALLOCATION_FAILED;
        }

//      fprintf(fileOutput, str);
    }

after this loop I simply return an enum stating success.

Here are the errors in Valgrind:

==8779== Use of uninitialised value of size 8
==8779==    at 0x4037C2: UserGetName (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x401FAC: SuggestFriends (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x402E6D: executeUserCommand (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x40281B: main (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779== 
==8779== Use of uninitialised value of size 8
==8779==    at 0x4037A0: UserGetID (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x401FC8: SuggestFriends (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x402E6D: executeUserCommand (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x40281B: main (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779== 
==8779== Invalid read of size 1
==8779==    at 0x403F1A: mtmSuggestFriends (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x401FEE: SuggestFriends (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x402E6D: executeUserCommand (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x40281B: main (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==  Address 0x9848B4458BB44589 is not stack'd, malloc'd or (recently) free'd
==8779== 
==8779== Process terminating with default action of signal 11 (SIGSEGV)
==8779==  General Protection Fault
==8779==    at 0x403F1A: mtmSuggestFriends (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x401FEE: SuggestFriends (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x402E6D: executeUserCommand (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779==    by 0x40281B: main (in /u1/023/mtm/ex2/RUN/mtm_isocial)
==8779== 
==8779== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 4 from 1)
==8779== malloc/free: in use at exit: 1,250 bytes in 93 blocks.
==8779== malloc/free: 455 allocs, 362 frees, 10,081 bytes allocated.
==8779== For counts of detected errors, rerun with: -v
==8779== searching for pointers to 93 not-freed blocks.
==8779== checked 122,512 bytes.
==8779== 
==8779== LEAK SUMMARY:
==8779==    definitely lost: 0 bytes in 0 blocks.
==8779==      possibly lost: 0 bytes in 0 blocks.
==8779==    still reachable: 1,250 bytes in 93 blocks.
==8779==         suppressed: 0 bytes in 0 blocks.
==8779== Reachable blocks (those to which a pointer was found) are not shown.
==8779== To see them, rerun with: --show-reachable=yes

The function ToStringUser returns a malloc of const char*.. So I shouldnt worry about freeing it right?

Any idea why is this happening?

I tried to free the str with this code in the for but I keep getting the same errors and the same amount of memory leaks:

free((char*) str); OR free((void*) str);

Here is the struct of the User and the getID and getName:

struct User_t {
    char *Name;
    int ID;
    int Birth;
};
int UserGetID(User user) {
    return user->ID;
}
char* UserGetName(User user) {
    return user->Name;
}

Before the loop I initialize a new User with this:

User user = FindUserPointer(setUser, id);

The function used is this:

static User FindUserPointer(Set users, int ID) {
        assert(users!=NULL);
    User tmpUser = UserCreate("temp", ID, 99);
    if (tmpUser == NULL) {
        return NULL;
    }
    SET_FOREACH(User,Itrator1,users) {
        if (UserCompare(tmpUser, Itrator1) == 0) {
            UserFree(tmpUser);
            return Itrator1;
        }
    }
    UserFree(tmpUser);
    return NULL;
}
12
  • why wouldn't you worry about freeing it?
    – msam
    May 4, 2012 at 6:53
  • 5
    You should always worry about freeing memory.
    – Philip
    May 4, 2012 at 6:55
  • 1
    What is user in the for loop? And the program segfaults in the mtmSuggestFriends function, so it might be useful to see that.
    – huon
    May 4, 2012 at 6:57
  • @msam even if I free it by casting str to void* or char*, I keep getting these errors and the 93 block of memory leak.
    – Omar
    May 4, 2012 at 6:57
  • 1
    @Omar, The program has crashed. Your leak is another problem.
    – IanNorton
    May 4, 2012 at 7:07

3 Answers 3

5

Valgrind isn't complaining about a leak - it's complaining that you're reading uninitialized memory and dereferencing an invalid pointer (the invalid pointer deref is crashing the program - at least under Valgrind).

We'd need to see UserGetID() and UserGetName() to have a hope of determining the bug in those (but that still might not be enough).

Based on your comment that mtmSuggestFriends is an object file that you don't have source for, my guess is that UsetGetID() and/or UserGetName() are passing invalid pointers to mtmSuggestFriends.

5
  • Question updated with the code. the mtmSuggestFriends is a function in an object file. it mallocs and returns a const char*. Its not the cause of the leak as many of my friends used it and had 0 leaks when using it.
    – Omar
    May 4, 2012 at 7:09
  • I updated the for loop with the asserts (see code above) and its still working.
    – Omar
    May 4, 2012 at 7:17
  • Now we know more, but don't know what the data passed in to FindUserPointer() looks like or what SET_FOREACH() is. But based on your description of mtmSuggestFriends, the problem might be that the heap is corrupted. Maybe UserCreate() has a bug (maybe not allocating enough memory to include the null terminator on the Name string?). May 4, 2012 at 7:21
  • @Omar: step through the loop in a debugger. I'll bet at some point name ends up being non-NULL but pointing to nonsense/invalid memory. That will still pass the assert(), but won't make mtmSuggestFriends() happy. May 4, 2012 at 7:25
  • I did that, and I found out that the test we got wanted more users than I actually had so I had to check the recieved user_count to the max number I could print (and not more).. So I was going out of the FinalArray bounds. I added a small if(..), and now I dont have memory leaks nor errors.Thanks for the help Michael.
    – Omar
    May 4, 2012 at 7:50
0

First, you are passing in an unassigned pointer user. Then your SuggestFriends() function called from UserGetID() is using this garbage pointer full of randomness as a real pointer which leads to the invalid read (SEGV)

You might find that setting "warnings as errors" (-Werr on gcc) would probably show you where you are doing unpredictable things.

3
  • I updated the code with the user assigning. I debbuged the code with eclipse and user is not NULL when reaching this error.
    – Omar
    May 4, 2012 at 7:10
  • exactly, it is not NULL, it is a value that is not a valid pointer, probably from where valgrind complains about use of the uninitialized variable. You should ALWAYS initialize pointers to NULL.
    – IanNorton
    May 4, 2012 at 7:12
  • I tried with this: User user = NULL; user= FindUserPointer(setUser, id); Is this what u mean? I still get the same error though. I also tried to return a COPY of the user in the FindUserPointer, but still the smae.
    – Omar
    May 4, 2012 at 7:20
0
struct User_t {
char *Name;
int ID;
int Birth;
};
int UserGetID(User user) {
    return user->ID;
}

...and where is User defined ?

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.