- Feature Articles
- CodeSOD
- Error'd
- Forums
-
Other Articles
- Random Article
- Other Series
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
WWWWWWWWTTTTTTTTTTTFFFFFFFFFFF!!!!!!!!!!!!!!!!!!!
Admin
The the WTF is the name doesn't match the function? I understand that false in C is 0, and true is non-zero. So he returns true is the person in not a special merchant when the function is called is_special_merchant.
Admin
"The fact that this is still in production and provides completely random results is also a mystery."
How is a fact a mystery?
Alex, you really need to proofread more. :)
Admin
Is it fair to say that he wasn't so much a legend in his own time as a legend in his own mind? Or is he a legend in the same way that Beowulf's Grendel is a legend?
Admin
(Hey, I almost *fist*!!)
First of all, welcome back Alex.
Secondly, what a wonderful WTF!! Have you been saving this up for us? The best part of this is that it's prolly called once for each transaction. Absolutely glorious.
Hill
Admin
Hm... well, i really fail to see a WTF here. Fine, it reads no more than 10000 bytes, and it doesn't need to read that much to begin with. So ok... what else?
Admin
Haha. Nice.
And who'd have thought that we'd see the day when not having a FileNotFound return code might actually be a WTF?
Admin
Follows UNIX-style exit codes maybe? I don't know, I'm stretching it a bit here.
Admin
"Every merchant has a four-digit merchant number". Therefore, unless we assume all the four-digit merchant numbers start with 000 through 002, this function is consistently indexing into uninitialized chunks of array.
Admin
First, it's really enterprisey, brillant, works as coded, and all that other jazz.
Now....
Actually, it doesn't read in 10k bytes ... it only reads-in the 23 contained in the file. However, this scheme would need to be redone if they ever have more than 10k merchants. The concept isn't too bad of an approach, but the implementation missed the mark.Um, this is where I think the WTF is for this code segment. "Still in production" because it's just too enterprisey. :) But the fact that it provides completely random results should be no mystery to a decent C/C++/{anything similar} programmer/software developer/Q&A tester. I'll leave it to the reader to figure out why it's not a mystery.
[Note from Alex: this has since been rephrased for clarity]
I like how the return logic is negated. :P
(0 if special merchant and 1 if not a special merchant )
so, he really should have done one of the following:
int might_not_be_a_special_merchant(...)
int special_merchant_not_found(...)
inline int paula_is_brillant(int true_or_false_or_file_not_found) { return("Brillant!"); }
Admin
He's reading beyond the end of the loaded file with any ID larger than the config, thus causing an undefined behaviour. If he wanted to guarantee he didn't cause it, he'd use
if(fln <= (size_t)merchnum) return(1);
instead of
if(fln == (size_t)0) return(1);
Admin
Yeah, seems off, because:
if (is_special_merchant(myMerchant)) {
//whatever
}
will evaluate to true if the myMerchant is NOT a special merchant.
Not to mention 23 bytes for a 4-digit merchant code? So what happens when myMerchant = 2423?
Admin
The nice-guy inside of me says we should give the guy the benefit of the doubt and presume the 23-byte file was his "test" file to make sure his function works as coded, and he just got lazy and forgot to put in the data for the other several thousand merchants.
The evil-genius inside of me says he's decent fodder for my next crusade to take over the world.
Admin
The real WTF is that he declared a pointer for the file and didn't initialize it to null.
Admin
No, this approach is perfectly acceptable since he doesn't "use" the variable prior to it getting set appropriately later (in the call to fopen).
Admin
Easy, since in C these are always offsets, it starts at the beginning of the array and moves 2423 bytes forward. Who cares where the array ends. If they value saved at that location is a Y then the merchant is a special case. You can't control things outside of your defined bounderies so why allow access to them.
But here's something I never have been able to figure out. What if your array was placed in memory near the end or you memory space and code like this tried to access beyond your physical memory limits? What would happen then? This behavious is so undefined to be a true hardcore WTF with no reliable result.
Captcha: Fo' Shizzle
Admin
<sarcasm>
Sorry, I forgot my sarcasm tags
</sarcasm>
Admin
Well, let's see here.
1. No attempt at returning any kind of error codes. If it can't open the file, then nobody is a special merchant.
2. The same file is opened over and over again for every single line/transaction. With caching I guess this isn't a HUGE deal, but still it's horribly inefficient.
3. Any merchantnum > 22 will return 0 or 1 depending on what "garbage" is in the allocated memory. Odds are heavy it will be 1, but if there just happens to be a "Y" there...
5. ab is declared for no reason.
6. Any merchantnum > 10000 will just simply core dump/IPF depending on operation system.
Beautiful.
Admin
And given that there are several thousand merchants - who is volunteering to type in the configuration file? Never mind the implementation - the design is already a WTF.
Admin
W[ho]TF signed off on writing the software for maintaining this special merchants file instead of forcing it into the DB with all other merchant information?
Oh, and this code appears to be an approximate replacement for return(random()&0x01);
--DK
Admin
And also, of course, if the file is empty he'll be leaving it open and leaking a filehandle.
Admin
Another WTF right there. One could easily figure this out (at least what will happen as a result from a specific compiler/linker).
Admin
Not to mention that he only closes the file if everything succeeds.
looks like a problem to me.
Admin
Yeah, looks like he's following old skool UNIX exit codes. It's not really a stretch if you turn the wayback machine to the late 70's/early 80's. I'm more offended by the magic cookies and hardcoded pathnames. And the lack of bounds checking. And any number of other things that even 20 years ago would have made this a WTF.
But the real WTF is that companies continue to hire "senior" programmers like this who haven't kept up with language changes. I recently worked with a "senior" toolsmith who clearly wasn't comfortable with any modern languages. I finally asked what language he was most comfortable with. Like Scott here, he chose C. But not just any C. He was most comfortable with pre-ANSI C. Anything after pre-ANSI C, and he was pretty lost. WTF?
Admin
That's *one* of the WTFs here.
Here are some others:
Admin
Not necessarily an invalid assumption. He wrote the function, so he gets to define the assumptions too.
um, 4. .... ?? profit ??
I think "ab" comes from a cut-and-paste snippet from some earlier code where he was doing something similar and needed a variable named "ab" to represent the absolute merchant number for those odd merchants with a negative number. :P
or simply get garbage (i.e., see #3)
Admin
Lot of fun if the file is empty, it won't get closed.
Admin
Since he's allocating the 10000-element array on the stack, the processor will make sure that array never extends beyond the end of the physical memory space: if the stack is blown like that, you'll almost certainly get a processor interrupt and the OS will kill the process. But he could create a 10000-element array on the stack and then get a reference to the 1,000,000th element of it. If the processor had hardware memory management, that'd probably trigger an interrupt that'd make the OS kill the process; if it didn't, my guess is that it'd wrap around into low memory and read something from the OS code.
Admin
Yeah, you can just keep on poining out problems with this function. Reminds me of those "kiddy" exercises you find at the back of the comics pages in the local newspaper. "How many things can you spot wrong with this function?"
Admin
Whoops. I pointed out something else and deleted it. But forgot to update the sequence. Hasty posting will get you every time.
Yeup..you're right. I forgot that if you're LUCKY you will get a coredump in C. The 10,001st element, for instance, may in fact give you the value of one of the stack variables...
Admin
The above code is so object-oriented, I almost OOPed my pants.
Admin
Trivial wtf next to the rest, but 7 Y in 23 charaters is 30.4%. And isn't he lucky the Y's are all within the first 23 merchant id's, 0000 thru 0022.
Also, if merchants are 4 digit the max is 9999, and indexing won't overflow the 10000 array length.
Admin
The array is allocated on the stack. So, on 32-bit x86, ((int)filedat[10000]) will be the same as fln, ((void)filedat[10004]) will be the previous value of of ebp, ((void)filedat[10008]) is the return address of the function, ((int)filedat[10012]) is the parameter merchnum, followed by the locals of whatever called is_special_merchant, and so on. If the index is high enough, it will wrap around and could end up pointing anywhere. If the address is not mapped (or is part of the protected kernel address space) it causes a segmentation violation, which on Windows is represented by that nice little dialog box asking you to send an error report to Microsoft. Unless this code is being run in the kernel (which itself would be a major WTF), then it would cause a panic or bluescreen.
Admin
That would likely cause a memory-access-violation.
However, I believe that this legend also had another function that just happened, by design, to have 10K worth of Y's and N's on the stack, just before this function gets called (every time), so that when it indexes off the end of the array, the data is valid, albeit by chance (I've actually seen this done in production code, and when confronted, the developer said that it worked, so why bother changing it)
Admin
That right there is the real gem. Not the implementation, but the fact that this code even exists. This information should, nay MUST, be in the database along with all of the other DATA. There's no reason in this universe that a special field should be put outside of the rest of the data.
Even the most perfectly coded, logical method for determining the "special" nature of the merchant would be wrong - it's not the correct place to put this sort of information!
His code's wrong, though.
Admin
Did no-one else catch the other WTF here?
if ( ! is_special_mercant(merc_id) ){
// DO SPECIAL MERCH STUFF
}
He returns 0 for success and 1 for failure!!! WTF?!
Admin
Good catches.
He probably had an accompanying utility function to handle this case:
void init_stack_for_is_special_merchant() {
char stackData[20000];
int i;
for(i = 0; i < 20000; ++ i) stackData[i] = 'Y';
}
:)
This isn't really a problem if the merchant ID is a 4-digit number like the problem statement says it is. Of course, it would be a problem if they ever needed more digits, but, judging from the style, this constraint is probably hard-coded in many more places than just this function. :)
You got all the errors I could see, except for the leaked file handle if the file is empty.
Admin
Granted, that's one screwed up way of doing it, but a 3 line shell script could be used to create the file, then edit in vi, then enter: 3048 <right arrow> rY<esc> (not that you'd want to do it that way, but you could)
Admin
In the 70's speed a space were such an importance...i mean i'm not defending the guy but:
If you knew that all the special merchants would be near the front, this is actually pretty fast, and for his time-frame that's pretty configurable to have a file like that. You have to assume the merchants don't change that much, i mean it's in a flat file after all.
His only wtf was the char[10000] part, which is what gives you random data. the fread doesn't jsut read random data past the end of file marker, but the char array will already have the random data in there. What he should have done was check to see if merch_code > filesize and return not special for that case.
A better approach in this case would to have had a directory of special merchants, with 1 file for each special merchant named with the merchant code. So, eg if merchant 5678 was special, there should be a 5678.merchant file in the special_merchants directory. You try to open up the file for reading, and if it works you know it's special, otherwise it's not. Very simple to add special merchants, and you could even go as far as putting security on merchants via the filesystem.
obviously a layer above this for cacheing would even be better, but you guys are all assuming that this function is called multiple times per an execution pool. For all you know this is a CGI that is activated every time a request comes in, meaning there is nowhere to cache results.
Admin
I actually format my pointers before I use them. Not quick format, but the Real Thing! I also double check that the pointer's master boot record is in place, and run S.M.A.R.T. to ensure that it doesn't have any bad sectors or sumfin. You can never not know.
Admin
Not necessarily; you are thinking in decimal. This guy is a legend in his own mind - maybe he thinks in hex, so 4 digits would be: 0..63K-1</smirk>
Admin
Somehow I don't think this code is as big of a WTF as being claimed, or if it's a WTF at all.
Merchant codes are at max 4 digits, meaning the largest merchant code would be 9999. He allocated 10000 bytes for his buffer. Perfect. It works perfect for the current spec. fread attempts to read that many bytes. It'll read as many as it can, so good. He doen't check that the merchant code is greater than the length of the file. That COULD be a wtf.... No, frankly this code isn't that bad. Sure, if it encounters an error it'll return 1, but that's the worse it'll do. Just make sure the mechant codes start at 0 and increment from there.
Admin
actually, unless Alex made a typo and reversed the arguments to fread, this procedure will return completely defined results when confronted with a 23byte file, it'll claim that the merchant is not special, because the return value of "fread" will be 0
Admin
It will cause a perfectly reliable segfault. (Although it's the address space allocated for the process and not the physical memory that counts )
Admin
The short answer is: "You're farked."
If you're lucky, your program will stop running. Your watchdog timer (on an embedded system) will trigger and restart. Your computer will crash. You'll get a "not responding" error.
If you're not lucky, well, have you ever played "Memory rodeo"? It's where you and a friend each write a program to write random values to a random location in memory. That's what you'll end up doing. Remember that your peripheral devices, like your HDD, monitor, Ethernet card, etc. are all locations in memory. (At least down on the lowest levels.) If you write garbage to your monitor or to a random section on your HDD, what do you think is going to happen? As someone else said, "It's like your VCR manual: it doesn't say what happens if you insert a banana upside down in zero gravity. It's undefined."
If you're really unlucky, then you've created a buffer overflow exploit and your code gets released. There are errors in every piece of C code that uses "gets". There's no limit to the buffer size, and you get all kinds of malicious code potential.
That's the problem with C. It lets you fark up. Fark up big time.
Admin
Yeah, if you didn't catch what i was hinting at with random data: since he doesn't check that the merchant code is greater than the file length, (or 9999), he could pass back "random" data.... will most likely be 1's.
Admin
shit... good catch.
Admin
Ummm, yeah, lots of us caught it. Did you miss the 10 or so previous posts about it?
Admin
but his char[10000] won't be 10000 null char's it will be random, with one of them possibly being the bit codes for the char Y, and hence he'll get a false positive (or in his case a true negative?)
Admin
Argh... nice catch. Can't believe everyone missed that. I guess the big question then is: at what point does he run out of file handles?