• No Leak (unregistered)

    There's a return above the free() statements. It's dead code.

  • (cs)

    Most likely placed there after a 6-hour debugging session trying to figure out why the tree is disappearing 5 minutes into the application execution...

    Wow.

  • arctanx (unregistered)

    Who node why that became necessary....

  • Paul Nieuwkamp (unregistered)

    Deleting pointers, ok. Strage place to (try to) do that (better place would be the destructor), but let us humour the guy.

    But freeing native integers? What language is that?

  • PJH (unregistered)

    I think they forgot

    free( traverseGroup );

    ;)

  • medo (unregistered)

    C, it's been a while... but I think you can't even know if the variables are on the stack, because they are passed by pointer. So the code (which luckily never runs anyway) frees some variables of the calling function (which is of course idiotic but not necessarily wrong, i.e. the objects were only created for passing to the function), but also some random memory location (given in n) which would most probably cause an error from the memory manager at best.

    This is only my quick interpretation and I haven't coded in C for ages, so I could well be wrong.

  • PJH (unregistered) in reply to Paul Nieuwkamp
    Paul Nieuwkamp:
    Deleting pointers, ok. Strage place to (try to) do that (better place would be the destructor), but let us humour the guy.

    But freeing native integers? What language is that?

    I don't think they are trying to delete the pointers, they're trying to free up the local variables. (Which in C and C++, which is what I think this is, are auto anyway and self destruct on return from the function.) Note that the free()'s are in unreachable code anyway

  • fner (unregistered)

    I'm not a C-expert but what would 'free ( n )' do? It's an int. It would (try to) free some data on address n? (If there weren't the preceding returns.)

    Caotcha: burned - I was (many times)

  • fner (unregistered) in reply to fner

    Er... wanted to write CaPtcha...

    Captcha: tacos (burned by tacos - makes sense...)

  • Baggy McBagster (unregistered)

    Cargo Cult programming at its finest! Way back in the mists of time, probably someone who knew what he/she was doing told one of the clueless developers about memory leaks and fixed one by putting in a free().

    "Always free() whatever you allocate," she said, then left for her next contract.

    Ever since which, clueless developers have handed the wisdom down. With each generation the sense of ritual and awe increases. The code works perfectly too, assuming you don't do something dumb like place the return after the frees. And it prevents memory leaks! Are there any memory leaks in the above code? Nooooo.

    I have to take issue with the comment 'stack variables want to be free'. It's the heap memory pointed to by those stack variables that's being freed -- whether they're pointers or not.

  • skztr (unregistered)

    good point. He probably meant free( &n ); There. Much better.

  • wj (unregistered) in reply to No Leak

    When I saw the dead code below return 0;, I couldn't do anything but to sigh.

  • Cyrus (unregistered) in reply to fner
    fner:
    I'm not a C-expert but what would 'free ( n )' do? It's an int. It would (try to) free some data on address n? (If there weren't the preceding returns.)

    Caotcha: burned - I was (many times)

    Would this even work? n is not a pointer, stack or heap it doesn't matter. If the compiler was at all decent it would smack you for trying call free() with a non-pointer instead of "helping" you and assuming you wanted to use the int as a memory address.

    CAPTCHA: Muhahaha, indeed.

  • Anonymous (unregistered)

    Definitely looks like C code. I'm seeing:

    1. All free calls are short-circuited by the return
    2. The first two free calls are OK. They aren't freeing stack variables, they're freeing the Nodes that the arguments (which are most likely on the stack) point to. Not a good practice, but valid (if the Nodes were dynamically allocated in the first place).
    3. The third free call is going to deallocate the array of pointers. It isn't going to deallocate the Nodes pointed to by the contents of that array. Probably not what they want to be doing here, but once again valid if the array of nodes was dynamically allocated (which is definitely possible if the size isn't known).
    4. The last free call looks like a no-no. The only way this could possibly be valid at compile-time was if n were implicitly casted to a pointer. I don't think a single C compiler I work with would do this without over-riding a whole bunch of checks. At run-time it's only valid if n was a pointer tom a dynamically allocated chunk of memory casted to an int in the first place. Just plain bad all around.
  • newt0311 (unregistered) in reply to fner
    fner:
    I'm not a C-expert but what would 'free ( n )' do? It's an int. It would (try to) free some data on address n? (If there weren't the preceding returns.)

    Caotcha: burned - I was (many times)

    the n was passed in by value as an integer.

    in C, there is no way to differentiate between ints and pointers. the result of free(n) depends on the situation. If during application runtime - those guys were so unlucky as to have actually allocated something at the memory address n, that address space would be freed up. If that was not so, there would be an error code returned except these guys were never checking the error codes any way so that program could continue as normal. i.e. they will have a random crash sometime and will probably never be able to find the problem. As a side note, the compiler should give a very big warning of there being no casts to turn an integer to a pointer... (I know gcc gives the warning. don't know about what they are using but if it is anything halfway decent, it should give a very big warning.)

  • newt0311 (unregistered) in reply to skztr

    freeing pieces of the stack... way to go...

  • PJH (unregistered) in reply to Anonymous
    Anonymous:
    Definitely looks like C code. I'm seeing:

    [...] 2) The first two free calls are OK. [...]

    No they're not ok. Did you miss this bit of the specification at the end?

    It should be noted that this function is not supposed to change the data-structure
  • Scotty (unregistered)

    Assuming that this is C, and notwithstanding the return statement, free() is used to give back a section of the heap which was manually allocated with a standard C function like malloc(). Heap memory manually allocated in C is NOT given back automatically, and there is no garbage collector, so a common source of crashes is running out of memory due to a 'leak'. As a corollary, a common point beaten into the heads of C programmers is to be sure to release all allocated memory -- when you no longer need it. Hence the programming lore. So if the pointers passed in were originally obtained by a call to a heap allocation function, then the free() call would actually free up the memory. The middle of a traversal function doesn't look like a good place to do that, so that's a WTF in any case: who knows who else is using references to that chunk of memory. If on the other hand, the parameters are pointers to local variables or stack variables of some calling function, then the use of free(), were it actually to run, would result in either nothing happening (if the free function was implemented in such a way that it ignored addresses not within the heap) or some kind of exception. So that's another WTF. And if the programmer was actually thinking to free up the stack, as opposed to some heap, then he/she is so utterly ignorant of computer memory organisation as to constitute a WFT**2.

  • mpd (unregistered)

    The biggest, and hardest to debug, problem in this code is simply that based on what we've been shown traverseGroup doesn't allocate the memory it frees. (yes, there's a return before that but as someone else pointed out, it was probably placed there during debugging.)

    A cardinal rule of C coding is never free memory you (the function) don't allocate. (Yeah, free'ing the integer will also cause problems.)

    C++ has destructors but they don't this way. If you have something like this:

    class SomeClass { /* bunch of stuff */ };

    SomeClass *foo = new SomeClass ();

    You still must free foo with the delete keyword. The class's destructor will be called to give SomeClass a chance to free any memory it allocates, but the memory allocated by new must be explicitly freed or it's allocated until the program exits. The memory is NOT freed when the end of the block is reached.

    If, however, you have something like this:

    void func () { SomeClass foo; foo.blah (); return; }

    Then the memory is freed automatically because SomeClass is on the stack, not on the heap. Notice that I didn't use new or malloc() in that function. Therefore, I don't need to use free() or delete.

  • snoofle (unregistered) in reply to newt0311

    A long time ago, I worked with this guy who thought it was cute to do something like this:

    
/********************************************
 * very long comment                        *
 * blah blah blah */ x++; /* blah blah blah *
 * very long comment                        *
 ********************************************/

    Man, I hated that guy.

  • (cs) in reply to PJH
    PJH:
    Anonymous:
    Definitely looks like C code. I'm seeing:

    [...] 2) The first two free calls are OK. [...]

    No they're not ok. Did you miss this bit of the specification at the end?

    It should be noted that this function is not supposed to change the data-structure

    I should have clarified. They're OK syntactically and if the memory is dynamically allocated aren't going to shit the bed. Granted, some other code may take great offense to the data being freed up. It's valid, just not necessarily correct in-context.

  • William (unregistered)

    What happens when this sample code runs?

    int main() { Node myWilly; Node myDadsWilly; Nody childrensWillies[] = Node[2];

    return traverseGroup(&myWilly, &myDadsWilly, childrensWillies, 2);

    }

  • Thorfinn (unregistered) in reply to PJH
    PJH:
    I think they forgot 
    free( traverseGroup );
    ;)
    Also 
    free( status ); free ( errors ); free ( beer ); free( steak_knives );
    Mind you, at least the code shown to Mac should work. The free()s are just some obscure kind of bogus comment. ;-)

    Captcha: smile (it's a nuclear hand grenade)

  • Andy (unregistered)

    The line free(n) would work fine.

    If the numeric value of n happened to be the address of an allocated memory block. Otherwise "The results are undefined, but probably catastrophic."

    In the Good Old Days, integers were used to hold address. Therefore, the compiler has to allow integers to be used as pointers.

  • Thorfinn (unregistered) in reply to William
    William:
    What happens when this sample code runs?

    int main() { Node myWilly; Node myDadsWilly; Nody childrensWillies[] = Node[2];

    return traverseGroup(&myWilly, &myDadsWilly, childrensWillies, 2);

    }

    Demons may fly out of your nose.
  • another thought (unregistered) in reply to Thorfinn
    Thorfinn:
    PJH:
    I think they forgot 
    free( traverseGroup );
    ;)
    Also 
    free( status ); free ( errors ); free ( beer ); free( steak_knives );
    Mind you, at least the code shown to Mac should work. The free()s are just some obscure kind of bogus comment. ;-)

    Captcha: smile (it's a nuclear hand grenade)

    Take it to the extreme:

    free(status);
free(errors);
free(this); // pun intended
free(that);
free(theOtherThing);
free(free);
  • (cs) in reply to William
    William:
    What happens when this sample code runs?

    int main() { Node myWilly; Node myDadsWilly; Nody childrensWillies[] = Node[2];

    return traverseGroup(&myWilly, &myDadsWilly, childrensWillies, 2);

    }

    I assume you want to do this instead, as syntactically what you've got above isn't quite kosher:

    int main() { Node myWilly; Node myDadsWilly; Node childrensWillies[2];

    return traverseGroup(&myWilly, &myDadsWilly, &childrensWillies, 2);

    }

    Assuming you have a very lax compiler and you actually get a binary out of it, you'll probably crash in the section that assigns status. Assuming you trim that out, you'll return. Assuming you trim that out, it's up to the free implementation. You're trying to free memory on the stack, so chances are free is going to ignore it or do A Bad Thing (tm). Maybe it'll shit the bed now, maybe it'll shit the bed later, maybe it'll continue along blissfully unaware.

  • (cs) in reply to William
    William:
    What happens when this sample code runs?

    int main() { Node myWilly; Node myDadsWilly; Nody childrensWillies[] = Node[2];

    return traverseGroup(&myWilly, &myDadsWilly, childrensWillies, 2);

    }

    Now let's not go flying around all willy nilly ok.

  • (cs)

    linux = free(as_in_beer);

    g,d&r

  • Peter Antoine (unregistered)

    Incredibly stupid, but...

    int payload;

    /* ... some other stupid code ... */

    search_string = (int) malloc(....);

    memcpy(search_string,"hello moto",sizeof("hello moto"));

    traverseGroup(&myWilly, &myDadsWilly, &childrensWillies, search_string);

    ...this makes what they were doing STUPID but valid. (before the return 0, fixed it)

    PS: I have seen int and void* uses for some absolutely stupid reasons.

  • Thomas (unregistered)

    Why would this guy pass as an argument the parent and the children of the node? Shouldnt the data structure be organized in the way that you can actually track the nodes parent and his children having the node. Bad data structuring too.

  • man (unregistered)

    It's easy. As those pointers are arguments he just saw a comment saying something in the lines "don't forget to free those pointers after returning". And that he did.

  • Ownage Personified (unregistered) in reply to another thought

    some guy : "There ain't no such thing as a free lunch"

    Mac: free(lunch); "Now there is."

  • William (unregistered) in reply to Thorfinn

    Well, that's OK, as long as I get to...

    free myWilly

    You know, like in the film ;-)

  • justsalt (unregistered) in reply to another thought

    [/quote] Take it to the extreme:

    free(status);
free(errors);
free(this); // pun intended
free(that);
free(theOtherThing);
free(free);
    [/quote] 
    free(bird); /* ! */
  • mr. freedom (unregistered) in reply to justsalt
    justsalt:
    free(bird); /* ! */

    free(dom) anyone?

    Captcha: pinball (at least not that stupid onomatopoedia again!)

  • SomeCoder (unregistered) in reply to mpd
    mpd:
    A cardinal rule of C coding is never free memory you (the function) don't allocate. (Yeah, free'ing the integer will also cause problems.)

    Yeah, tell that to Microsoft in their MFC libraries frustrated sigh

  • PC Paul (unregistered) in reply to snoofle
    snoofle:
    A long time ago, I worked with this guy who thought it was cute to do something like this: 
    
/********************************************
 * very long comment                        *
 * blah blah blah */ x++; /* blah blah blah *
 * very long comment                        *
 ********************************************/

    Man, I hated that guy.

    Syntax highlighting and comment folding are your friends...

    I worked with a new graduate once who came in to our 800K lines+ C codebase and spent his first week figuring out a find/sed script to strip out all the comments, because they 'got in the way'.

    We had a little talk...

  • Alistair (unregistered)

    I think I may know why, I've written similar code too.

    They key thing to note is that it's dead code, never executed. Imagine you've got warning levels up so high that it issues warnings for unused local variables and unused function arguments. Now imagine that you've got "treat warnings as errors" turned on. As you're developing the code you may or may not use all of the function args, sometimes you may never use all the function args but they are there anyway because of architectural reasons. So you write dead code after the final return, something, anything, that does something to each and every function argument. Compiler is happy, optimizer then strips it all away again, developer is happy.

    Of course, that doesn't explain the wording of the comment or the use of free()! Whenever I write dead code like this I just assign each arg to itself:

    peer = peer; parent = parent; children = children; n = n;

  • orb (unregistered) in reply to mr. freedom
    mr. freedom:
    free(dom) anyone?
    free(keyshit)
  • John Kugelman (unregistered)

    The line free(n) would work fine.

    No, it wouldn't. C is strongly-typed. That's not to say you can't cram an integer where a pointer is asked for--this is C we're talking about, after all--but you have to at least pretend what you're doing is legit and add a (void *) cast.

    (Well, unless there's no #include <stdlib.h>, meaning free() has no prototype. But any compiler from the last 20 years should throw up a warning or two about that.)

  • Jeltz (unregistered) in reply to John Kugelman
    John Kugelman:
    The line free(n) would work fine.

    No, it wouldn't. C is strongly-typed. That's not to say you can't cram an integer where a pointer is asked for--this is C we're talking about, after all--but you have to at least pretend what you're doing is legit and add a (void *) cast.

    (Well, unless there's no #include <stdlib.h>, meaning free() has no prototype. But any compiler from the last 20 years should throw up a warning or two about that.)

    Nope, it works. C is not strongly typed in this case. You can pass an integer where a function expects a void *. The resulting error in gcc is:

    "warning: passing argument 1 of ‘free’ makes pointer from integer without a cast"

  • Jeltz (unregistered) in reply to Jeltz

    Obviosuly I meant "the resulting warning". :)

  • Shoop (unregistered)

    <quote>"Failed to traverse successfully"</quote>

    Classic! I vote for this to be added to the same list as the
    TRUE,FALSE,FILE_NOT_FOUND.

    <digression> Can someone take a snapshot of this code, print it out, place it on a table, take a picture of it, get the pictures developed, scan the picture in, and post it so we can...... </digression>
  • Marcin (unregistered)

    Learn C, you hippies.

    (Those of you who know C might as well go out for a smoke whole the rest of the class catch up.)

  • MrHen (unregistered)

    Not that this is incredibly minor compared to the rest of it, but I like that it returns 1 when it errors out. Very intuitive.

  • Marcin (unregistered)

    Learn C, you hippies.

    (Those few who know C might as well go out for a smoke while the rest catch up.)

  • Greenpeace (unregistered)

    free(willy);

  • Jeltz (unregistered) in reply to MrHen
    MrHen:
    Not that this is incredibly minor compared to the rest of it, but I like that it returns 1 when it errors out. Very intuitive.

    Well, this is a matter of taste and mostly lots of confusion. The main function is supposed to return 0 on success while generally other functions return 0 on failure. All functions having several error codes (like I think pthread_create) returns 0 on success and whatever code on failure.

    What C would have needed is more boolean values than two.

  • gallier2 (unregistered) in reply to Alistair

    peer = peer; parent = parent; children = children; n = n;

    We a project that was compiled with MS-C version <= 7.00 and Borland C (Turbo-C) <= 2.00 and both emitted warnings for unused parameters. Ms-C could be satisfied with your code (a simple perr; was even suffisant), but Borland was another beast, it knew that it wasn't used and warned anyway. I really had to do complicated things to get him quiet on this. At the end I didn't bother and let the warnings.

Leave a comment on “Mr. Memory Leak”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article