The Daily WTF: Curious Perversions in Information Technology

2007-02-06 Reply Admin

Baggy McBagster:
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.

You mean this: http://www.worldreligions.ca/blog/news/2006/10/gurus-cat.html

WIldpeaks · 2007-02-06 Reply Admin

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.

Ohhhh I'm so gonna re-use it one day to drive people nuts :-D

quephird · 2007-02-06 Reply Admin

At least he thought of the children.

2007-02-06 Reply Admin

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.

Returning zero on success and nonzero on an error is fairly standard for C code for situations where there's only one success condition and potentially several error conditions, certainly not even a minor WTF(although you may, of course, personally dislike the arrangement - standardised error handling isn't really C's strong point).

poochner · 2007-02-06 Reply Admin

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.

Holy Crap! Back in the bad old days when you didn't have a decent editor that would show you that kind of thing, that would be just freakin' evil. He deserved a serious smack upside the head.

2007-02-06 Reply Admin

MrHen: If the status is to be used as a unix exit code, it's correct to use 0 to mean success and non-zero to mean failure.

You guys keep talking about the compiler barfing... I don't even use C so I had no good excuse for not actually trying it :-)

I used gcc 4. If you don't pass -Werror, you get a warning but it produces an executable just fine. Which segfaults, of course :-) Now, pared down to nothing but the dead code:

#include <stdlib.h>

typedef char Node;

void traverseGroup( Node *peer, Node *parent, Node *children[], int n ) { free( peer ); free( parent ); free( children ); free( n ); }

int main() { Node *peer = "milton"; Node *parent = "bob"; Node *children[2] = {"michael bolton", "samir"}; int n = 2;

traverseGroup(peer, parent, children, n);
return 0;

}

pw@sweetums $ gcc wtf.c wtf.c: In function 'traverseGroup': wtf.c:11: warning: passing argument 1 of 'free' makes pointer from integer without a cast

pw@sweetums $ ./a.out Segmentation fault

But hey, that's easily fixed, we can get rid of that annoying warning by changing just one line:

  free( (char *)n );

There, problem solved!

pw@sweetums C $ rm -f a.out pw@sweetums C $ gcc -Werror wtf.c pw@sweetums C $ ./a.out Segmentation fault

In fact, we never get as far as free(n), it segfaults on the call to free(peer).

Moral of the story: -Wall and -Werror are your friends.

The other moral of the story: Even your best friend can't stop you from being a f***ing idiot.

gwenhwyfaer · 2007-02-06 Reply Admin

snoofle:
Man, I hated that guy.

I'm not surprised! That's just evil... deceptive coding at its, er, finest.

gwenhwyfaer · 2007-02-06 Reply Admin

Marcin:
Learn C, you hippies.
(Those few who know C might as well go out for a smoke while the rest catch up.)

Trust me, that comment really wasn't good enough to post twice.

Or, for that matter, once.

mrprogguy · 2007-02-06 Reply Admin

Alistair:
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;

If it were up to me, I'd set the pragma to allow unused arguments, or just not name them (type, but not name).

Then I'd go find out why the call was designed with unused arguments, and beat some sense into the people maintaining it.

2007-02-06 Reply Admin

The thing I like is that they have the operands to == reversed, so they obviously have programmers who care about code quality and have maybe even read a few books on the subject. Yet somehow, they managed to avoid having the compiler warn them about dead code, or chose to ignore those warnings.

2007-02-06 Reply Admin

gallier2:
> 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.

I'm no professional, but wouldn't the obvious way to make the compiler shut up be either commenting out the declaration of the unused variable, or even deleting it altogether if it's not going to be used?

Those compilers have become so smart these days, that "using" the variable after a return statement will just result in "dead code" warning at a sufficient warning level.

By the way, what kind of a data structure are we dealing with in the first place. How come parent and children have separate pointers that can be accessed and freed independently? How can parent be freed before children (assuming they are somehow related). And wouldn't it normally be a parent's responsibility (and purpose) to know where the children are?

2007-02-06 Reply Admin

Oy! OK, I'm not going to point fingers, but some people have forgotten their C pointers.

Node aNode; Node nodeArray[] = { /* whatever */ };

&aNode is a Node* nodeArray is a Node* &nodeArray is a Node** &nodeArray[0] == nodeArray, and they're both Node*

2007-02-06 Reply Admin

PseudoNoise:
Oy! OK, I'm not going to point fingers, but some people have forgotten their C pointers.
Node aNode; Node nodeArray[] = { /* whatever */ };

&aNode is a Node* nodeArray is a Node* &nodeArray is a Node** &nodeArray[0] == nodeArray, and they're both Node*

Finger pointer! Hah, I kill myself

captcha: alarm. Always disable

2007-02-06 Reply Admin

This is all just a trick so that when they are debugging, they can move the PC down to the unreachable code, slap some values in the variables, and use the debug step function to free up whatever they need, then reset the PC back into the main logic of the function. Continue.

Actually, I think the compiler warning threads are probably a better bet.

Thank goodness the IDEs I have been using the last 8 years allow you to type arbitrary code whenever and wherever you want it. Highlight, right click, "Execute".

Whippersnappers just don't know what it was like in the old days. Okay, I have to admit it, I missed using punch cards in school by one year.

danixdefcon5 · 2007-02-06 Reply Admin

Nooo that code was wrong!

It should be

free ( freddie_mercury );

Because, he wants to BREAK FREEEEE!!!

Hm... somehow I remember my data structure class. I still remember those dudes that free()d the root element from an AVL tree. Oops!!!

2007-02-06 Reply Admin

Alistair:
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.

If you have unused local variables, delete them.

If you have unused function arguments, remove them if that makes sense, otherwise do this:

int useless_function(int useful_arg, int /* useless_arg */) { return useful_arg+1; }

And the compiler will be happy. And no ugly hacks.

cklam · 2007-02-06 Reply Admin

Cyrus:
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.

Ooooooooohhhhhhhhhhhhh - back way when the early C compilers in SUNOS 4.1: one could dance samba on the keyboard whilst running vi, save the resulting file with .c -extension and it would compile .... sometimes you'd get a warning, though.

So this is maybe really code ...

2007-02-06 Reply Admin

Ah, I guess he was also a master of some of the techniques listed here: http://thc.org/root/phun/unmaintain.html ? :)

Namely, the "Camouflage" section.

cklam · 2007-02-06 Reply Admin

PC Paul:
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...

Did you check the size of the code base with all comments stripped ? Might have had something there.

Even with syntax highlighting: the example is almost evil and surely grossly negligent.

IMHO, comments are like speeches at family functions - unimportant for most of the rest of humanity and the shorter the better.

I usually omit them - they mar the beauty of pure code.

savar · 2007-02-06 Reply Admin

Jeltz:
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.

The convention comes from Unix, where a process could return any of a number of error codes. Zero is a convenient standard for "no error", and since main() is the developer's entry point in most cases, main() usually follows the same standards.

The confusion comes when you start defining functions that create objects. Now its more convenient to return zero as failure (null pointer) and non-zero as success (pointer to object) -- especially in languages where the runtime provides an exception handling mechanism so that error codes aren't needed anyway.

This is why we name constants...instead of "if (ret_value)..." we say "if (ret_value==kSuccess)" or something similar.

darin · 2007-02-06 Reply Admin

If they were trying to free the local variables, they should have passed pointers to them. As in: free( &peer ); free( &n );

Free() is not the same as the delete operator.

merphle · 2007-02-06 Reply Admin

Jeltz:
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.

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.

Interestingly, fork() works the same way -- though I've never been able to find out what each of the error codes means... I always just run fork() in a while loop until it returns zero.

2007-02-06 Reply Admin

Let's say the developer wasn't clueless about how to properly free up memory allocations and how not to create dead code, and say he coded this function to work exactly as he intended. He would have created a function named "traverseGroup" (which walks a tree presumably) that has the SIDE EFFECT of deallocating the tree it just traversed. That's a big WTF right there!

2007-02-06 Reply Admin

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.

ouch, that's just evil

2007-02-06 Reply Admin

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.

This is a common misconception. C is statically typed, not strongly. Variables can only hold one type, the type they are declared as. C is actually weakly typed, because things like this are completely valid:

int foo = 5;
float* bar = (float*) foo;

i.e. casting can be done between any types, even between pointers and non-pointers. Whether the results are in any way useful is highly unportable and sometimes undefined - pointers and ints aren't necessarily the same size, after all - but syntactically and grammatically it's perfectly fine. (In contrast, languages like Lisp and Ruby are dynamically and strongly typed; variables can hold any type but one type can't be cast to another.)

The main problem with the original code is indeed free(n), of course. Again it's syntactically valid, although the compiler would likely issue a warning about passing free() an argument of the wrong type. But it's semantically suspect, because except in really pathological cases, n will not happen to correspond to a memory address that was allocated by malloc(). That's a sure-fire way to get a segfault (or the equivalent on the system at hand).

darin · 2007-02-06 Reply Admin

newt0311:
in C, there is no way to differentiate between ints and pointers.

Almost all C compilers will flag this as a warning. It's allowed in straight C though.

So the programmer also exhibits the common mistake of ignoring compiler warnings.

2007-02-06 Reply Admin

Dwayne Hicks:
The main problem with the original code is indeed free(n), of course. Again it's syntactically valid, although the compiler would likely issue a warning about passing free() an argument of the wrong type. But it's semantically suspect, because except in really pathological cases, n will not happen to correspond to a memory address that was allocated by malloc(). That's a sure-fire way to get a segfault (or the equivalent on the system at hand).

I should add, free(&n) won't work either. n is allocated on the stack, not the heap. The only valid arguments to free() are addresses previously allocated with malloc(), and neither &n nor (probably) n fit the bill.

I think the most probable explanation is an inexperienced programmer who didn't quite understand what it means to "free your memory".

2007-02-06 Reply Admin

merphle:
Jeltz:
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.

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.

Interestingly, fork() works the same way -- though I've never been able to find out what each of the error codes means... I always just run fork() in a while loop until it returns zero.

um... actually, fork returns the child PID to the parent process and 0 to the child process. running it in a loop would lead to infinite spawning. probably not something you want.

2007-02-06 Reply Admin

PseudoNoise:
&nodeArray is a Node**

No, it is not.

newfweiler · 2007-02-06 Reply Admin

Marcin:
Learn C, you hippies.

Oh, man, what's your hangup? You're bummin' my trip. Like, you're too heavy. Lighten up. Tune in, turn on, drop out. Stop being a shill for The Man. Peace.

2007-02-06 Reply Admin

newt0311:
merphle:
Jeltz:
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.

Interestingly, fork() works the same way -- though I've never been able to find out what each of the error codes means... I always just run fork() in a while loop until it returns zero.

um... actually, fork returns the child PID to the parent process and 0 to the child process. running it in a loop would lead to infinite spawning. probably not something you want.

I think that was a joke. :)

2007-02-06 Reply Admin

The real WTF is that the function has two exit points, and he only cleans up after the second exit point.

joe_bruin · 2007-02-06 Reply Admin

slinkp:
Now, pared down to nothing but the dead code:
#include <stdlib.h>

typedef char Node;

void traverseGroup( Node *peer, Node *parent, Node *children[], int n ) { free( peer ); free( parent ); free( children ); free( n ); }

int main() { Node *peer = "milton"; Node *parent = "bob"; Node *children[2] = {"michael bolton", "samir"}; int n = 2;
traverseGroup(peer, parent, children, n);
return 0;
}

In fact, we never get as far as free(n), it segfaults on the call to free(peer).

This crashes because you're trying to free statically allocated memory. More specifically, you're trying to free memory not given to you by malloc/calloc/realloc, which is an error on your part. Now, despite all of the WTFs in the article, we don't specifically know he's doing this particular one (congratulations, you've managed to add one). Once you fix this, then you can proceed to the undefined behavior of trying to free(n).

darin · 2007-02-06 Reply Admin

mpd:
A cardinal rule of C coding is never free memory you (the function) don't allocate.

It's probably better to not restrict this to "functions", but to "modules". Ie, the tree handling module is responsible for allocating and freeing nodes for the tree, even though the functions that grow or shrink the tree are not the same. "Module" can be a class, component, layer, etc.

It's also not a rule for "C". The rule applies to any language that has dynamic allocation - including C++ (though some C++ code can bypass this by inefficiently copying data as its passed around). Any exceptions have to be well documented.

2007-02-06 Reply Admin

mr. freedom:
justsalt:
free(bird); /* ! */
free(dom) anyone?

I'd rather have a free(sub)...

darin · 2007-02-06 Reply Admin

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

In modern days, we still have to use numeric addresses as pointers, and the compiler has to allow it or we're forced to create a new language that does allow it. How else do you modify the register that exists at address 0x80004020 without turning that integer into a pointer?

darin · 2007-02-06 Reply Admin

Marcin:
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.)

Not a good idea. Our beards might catch on fire.

darin · 2007-02-06 Reply Admin

mrprogguy:
If it were up to me, I'd set the pragma to allow unused arguments, or just not name them (type, but not name).

Pragmas are not portable. There is not pragma for this sort of thing in some compilers, and even if they all had one, you'd have an amazing mess of ifdef's.

Also, the "var = var" style does not always work; such as when var is a constant. Even the syntactically correct "var;" statement doesn't always remove the "unused parameter" warning. I've had compilers complain if I leave off a parameter name, so that technique doesn't always work either.

There isn't a portable solution to get rid of the warnings. The tedious solution is to change the compiler options (thus pushing the problem off to whoever maintains the build systems).

merphle · 2007-02-06 Reply Admin

Captain Obvious:
newt0311:
merphle:
Jeltz:
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.

Interestingly, fork() works the same way -- though I've never been able to find out what each of the error codes means... I always just run fork() in a while loop until it returns zero.

um... actually, fork returns the child PID to the parent process and 0 to the child process. running it in a loop would lead to infinite spawning. probably not something you want.
I think that was a joke. :)

Thank you, Captain Obvious! (wow, never thought I'd have a completely valid reason for saying that!) ;)

2007-02-06 Reply Admin

mr. freedom:
justsalt:
free(bird); /* ! */
free(dom) anyone?

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

That one's all over the place: http://www.google.com/codesearch?q=%22free%28dom%29%22&hl=en&btnG=Search+Code

2007-02-06 Reply Admin

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)

2007-02-06 Reply Admin

Paul Nieuwkamp:
Deleting pointers, ok. Strage place to (try to) do that (better place would be the destructor)

In C++, yes...

Paul Nieuwkamp:
But freeing native integers? What language is that?

But I'm afraid that is pure C, with as many errors and warnings disabled as possible (what are they good for anyway?).

free(n) will just push the value of n on the stack and call free. Who cares if it's a pointer at all? If this code was ever live, it should have caused quite a number of segfaults or aborts.

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?

Typically, yes, unless ints and pointers aren't the same length, in which case even funnier things can happen. (with funny as in "this smells funny")

2007-02-06 Reply Admin

AdT:
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)

I'm sorry, I posted the wrong thing and now I can't delete it. :-(

Quinnum · 2007-02-06 Reply Admin

Michael:
mr. freedom:
justsalt:
free(bird); /* ! */
free(dom) anyone?
I'd rather have a free(sub)...

free(YourMind);

free(Hat);

free(Coke_With_Your_Burger);

etc.. ;-)

2007-02-06 Reply Admin

Quinnum:

free(YourMind);

free(Hat);

free(Coke_With_Your_Burger);

etc.. ;-)

free(lunch);

Error: No such thing.

2007-02-06 Reply Admin

My eyes, they burn!

2007-02-06 Reply Admin

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.

I had to read that comment twice to catch that (the first time through I thought you were complaining about his "very long comment"). What a bastard!

2007-02-06 Reply Admin

Blah. This is why no one uses C any more.

foxyshadis · 2007-02-06 Reply Admin

18281828459045:
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.
Returning zero on success and nonzero on an error is fairly standard for C code for situations where there's only one success condition and potentially several error conditions, certainly not even a minor WTF(although you may, of course, personally dislike the arrangement - standardised error handling isn't really C's strong point).

Maybe he meant the fact that there's already a perfectly good status variable that could be returned, and instead it returns 1 no matter what the error.

Alchymist · 2007-02-06 Reply Admin

gallier2:
> 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.

Ummmm. Why not just remove the unused variables ... or comment them out if they are associated with commented out code that you might re-instate (don't like it but some people use it for testing) ... or put them in a #ifdef block if you have some platform specific code #ifdefed elsewhere. I can't see any reason just to keep them.

Edit: Oops - I was thinking you were discussing local variables. Teach me to look at this site so late.

Mr. Memory Leak

Leave a comment on “Mr. Memory Leak”