-
Hey, guest user. Hope you're enjoying NeoGAF! Have you considered registering for an account? Come join us and add your take to the daily discourse.
POP QUIZ #2: Find the bug in the following code.
- Thread starter aaaaa0
- Start date
- Status
Hitokage said:"Function" isn't in hungarian notation!
Har har.
True, but not the bug.Honestly though, I actually like Hungarian notation.
(Though I only use a subset of Hungarian notation, not all of it. Just the well known types. No cryptic lphwfstfsdhefhdgBlahs. And I only do it on variables, not functions.)
I know a lot of people hate Hungarian notation -- I never used to believe in it either, but once I got in the habit of naming all my variables with it consistently, it really does help when skimming through code, because at a glance I know if something is a pointer, or a pointer to a byte, or a pointer to a string, etc, without having to go back to the variable declaration or inferring it from what's being done.
The main thing with Hungarian is you have to be completely consistent in your use of it.
Badly written Hungarian notation is worse than none at all.
Ok, here's my serious submission. I fully expect it to be laughably wrong.
The program seems to be a sanity check, making sure that pb isn't larger than its buffer, and neither is pb or pb buffer larger than a predefined limit. However, given that everything checks out, both comparisons return TRUE and thus the function returns TRUE, except... a nonzero return value is an error code... then again, I do remember Windows being bizzare in this respect... so yeah... nevermind.
The program seems to be a sanity check, making sure that pb isn't larger than its buffer, and neither is pb or pb buffer larger than a predefined limit. However, given that everything checks out, both comparisons return TRUE and thus the function returns TRUE, except... a nonzero return value is an error code... then again, I do remember Windows being bizzare in this respect... so yeah... nevermind.
Hitokage said:
Well the function returns a BOOL. It returns FALSE if the pointer fails this test, and TRUE if it succeeds. I'll clarify the function name.
As a side note, most Win32 functions return BOOL as well.
(BOOL) TRUE returned from a Win32 API indicates success.
(BOOL) FALSE returned from a Win32 API indicates failure.
TRUE is defined as a non-zero value, and FALSE is defined as a zero value.
If you receive FALSE from a Win32 API you are supposed to call the API GetLastError() which will return a Win32 error code indicating the specific failure.
However GetLastError() returns DWORD, not BOOL. ERROR_SUCCESS is defined as (DWORD)0, and all Win32 error codes are defined as non-zero values, which may explain your confusion.
Lathentar said:
The function is trying to verify that pb is pointing to something within pbBuffer, assuming pbBuffer is 32KB in size and is valid.
While passing pbBuffer = NULL into the function is indeed an error (since a pointer with value NULL is always invalid), it is not the bug here.
You can assume the code calling this function will not pass NULL for pbBuffer.
There is no operator overload for comparison between BYTE* and BYTE* that will compare strings instead of the pointer's address values.
You can safely assume in this case that comparing the two BYTE* is comparing the addresses contained in each pointer.
Also, as a note, while BYTE* is indeed a typedef for char*, neither type guarantees the contents of the memory address pointed to contain a string.
You can safely assume in this case that comparing the two BYTE* is comparing the addresses contained in each pointer.
Also, as a note, while BYTE* is indeed a typedef for char*, neither type guarantees the contents of the memory address pointed to contain a string.
I wouldn't necesserily call it a bug, but the second comparison
(pb - pbBuffer < BUFFER_SIZE)
is definately unsafe code.
The substraction result of two pointers will be converted to whatever integral type your specific compiler implementation uses for the purpose - and that type size MAY(depends on the implementation) be smaller then the size of pointers, which in turn will break the code if the two pointers are too far apart.
To be on the safe side, you'd want to write
(pb < pbBuffer + BUFFER_SIZE) which compares the values as two pointers.
If there's other bugs, I can't see them right now :|
(pb - pbBuffer < BUFFER_SIZE)
is definately unsafe code.
The substraction result of two pointers will be converted to whatever integral type your specific compiler implementation uses for the purpose - and that type size MAY(depends on the implementation) be smaller then the size of pointers, which in turn will break the code if the two pointers are too far apart.
To be on the safe side, you'd want to write
(pb < pbBuffer + BUFFER_SIZE) which compares the values as two pointers.
If there's other bugs, I can't see them right now :|
maharg said:
The relevant C Standard passage is:
http://msdn.microsoft.com/library/en-us/vclang/html/_clang_Subtraction_.289629.asp?frame=true
But you are getting very close to the bug.
BugCatcher
Member
The function returns TRUE if pb is less than zero, but I don't know the reason for it.
For those who are trying to figure out why it behaves that way, maybe this will help:
if pb = -1 and pbBuffer = 1...
(pb >= pbBuffer) = TRUE // -1 >= 1 is true, hmm
(pb - pbBuffer < BUFFER_SIZE) = TRUE
((int)pb >= (int)pbBuffer) = FALSE
(pb - pbBuffer < BUFFER_SIZE) = TRUE
Just tried casting the pointers in my test program to see if that would change the result of the evaluation.
For those who are trying to figure out why it behaves that way, maybe this will help:
if pb = -1 and pbBuffer = 1...
(pb >= pbBuffer) = TRUE // -1 >= 1 is true, hmm
(pb - pbBuffer < BUFFER_SIZE) = TRUE
((int)pb >= (int)pbBuffer) = FALSE
(pb - pbBuffer < BUFFER_SIZE) = TRUE
Just tried casting the pointers in my test program to see if that would change the result of the evaluation.
rastex said:
And that's the solution right there.
If pbBuffer is 100, and pb is 3000000000 (both possible values when you have 4GB of address space), then when the subtraction is done, the result will be negative, which will pass the validity check, resulting in the function incorrectly returning TRUE instead of FALSE, and quite possibly causing your app to crash when it tries to use pb.
(Now, to be nitpicky, C officially does not define what will happen when two arbitrary pointers are subtracted from each other if they're not from the same allocation block, though most compilers will allow it, so you can argue this whole function was flawed from the very beginning for bonus points.)
BugCatcher
Member
So how do you rewrite the function to perform the same task, but without the bug?
The size of ptrdiff_t is implementation defined as well, so it could in fact be large enough to handle the difference between any two arbitrary pointers. You are, regardless, correct that this is a bug in any case, since:
Practically speaking, though, you can't expect it to hold more than the C library would ever give you in one allocation (malloc, stack allocated array).
At any rate, the real fix for this bug is to never use pointers in such a way that you're left guessing on matters like this. If you're left guessing on where a pointer goes, you're doing something wrong as far as I'm concerned. (certain exceptions apply: asserts, gaurding against stack overflows (but there are often better ways) -- especially in a context where privilege escalation is possible).
And yeah, the rather obscure ways signed and unsigned interact in C/C++ are one of the worst things about them as languages. Even the best programmers make mistakes like this quite easily.
it is undefined behaviour (C99 draft standard) in the event that ptrdiff_t is not large enough to hold it.
Practically speaking, though, you can't expect it to hold more than the C library would ever give you in one allocation (malloc, stack allocated array).
At any rate, the real fix for this bug is to never use pointers in such a way that you're left guessing on matters like this. If you're left guessing on where a pointer goes, you're doing something wrong as far as I'm concerned. (certain exceptions apply: asserts, gaurding against stack overflows (but there are often better ways) -- especially in a context where privilege escalation is possible).
And yeah, the rather obscure ways signed and unsigned interact in C/C++ are one of the worst things about them as languages. Even the best programmers make mistakes like this quite easily.
BugCatcher
Member
Oh, okay. Thanks.
maharg said:
You tend to see code that does checks like this in places where people are verifying input buffers from untrusted sources. Like in a network stack, operating system, etc.
So having to get it right is unavoidable. Someone somewhere has to write this kind of code.
Yup. The latest exploit tactic going around these days is the integer overflow/underflow attack. Lots of code out there has bugs in boundary checking routines almost exactly like this one:
Code:
// read the header of the file
ReadHeader( &pHeader );
// verify buffer is no bigger than we are expecting
if( pHeader->cElements * sizeof( AN_ELEMENT ) > MAX_BUFFER )
{
// OH NOS SOMEONE CORRUPTED OUR FILE!
return WHOAH_HEADER_IS_CORRUPT;
}... what happens if pHeader->cElements * sizeof( AN_ELEMENT ) > MAX_INT?
Nice excercise
Speaking of which though, IIRC C doesn't specify a requirement for pointers to be unsigned - so it might be possible a specific implementation could use signed ptrs for some obscure reason...
And of course, on something like a console without virtual memory, the original code, buggy as it is, would never have any problems (which is scary in whole different manner).
Speaking of which though, IIRC C doesn't specify a requirement for pointers to be unsigned - so it might be possible a specific implementation could use signed ptrs for some obscure reason...
And of course, on something like a console without virtual memory, the original code, buggy as it is, would never have any problems (which is scary in whole different manner).
Yeah these kind of bugs can be pretty horric, particularly since they don't tend to occur in predictable manner while testing.maharg said:
rastex said:
Here ya go - nothing very hard mind you, I just thought some people might find this cute.
Tell me what the following program does.
a) Find what the output of the printf statment will be, without running the code.
b) What is it that is implemented here.
If you cheat and compile the code, you only get half points
Code:
template<int n>
struct F1
{ enum { a = F1<n-1>::a * n };
};
template<>
struct F1<0>
{ enum { a = 1 };
};
template<int v1, int v2>
class F2
{ enum { a = F1<v2>::a,
b = F1<v1>::a*F1<v2-v1>::a
};
public:
enum { c = a/b };
};
//
//
printf("Result: %d \n",F2<7,12>::c);F1 computes a factorial. Each iteration multiplies N * (N - 1), and the template specialization of F1<0> is the termination condition for the recursive loop.
So F2 is calculating (12!)/(7! * 5!), but if that has any significance beyond that, I don't know it offhand.
And of course, the wonder of all this is that it's done statically at compile time without the language having a built-in factorial operator. The joy of templates and quasi-functional programming.
So F2 is calculating (12!)/(7! * 5!), but if that has any significance beyond that, I don't know it offhand.
And of course, the wonder of all this is that it's done statically at compile time without the language having a built-in factorial operator. The joy of templates and quasi-functional programming.
Spoiler tagged for those who don't know templates as thoroughly as I do.
Good idea about spoiler tagging, let's see if anyone else wants to have a go
maharg said:
Yup, this kind of stuff is basically an introduction to template metaprogramming. The things that can be done with it are quite fascinating, as a game programmer I can't help but be intrigued by any possibilities of improving compiler 'intelligence' so to speak.
- Status