Spot the Bug - August 14, 2005

re: Spot the Bug - August 14, 2005

who — Mon, 15 Aug 2005 09:13:58 GMT

dest[50] doesn't exist? :-)

re: Spot the Bug - August 14, 2005

Ceboz — Mon, 15 Aug 2005 09:24:34 GMT

I'm a little rusty but...

src is unitialized (and if the memory allocator doesn't initialise memory to 0's then strcpy may not work correctly).

We should not be copying from a char[100] to a char[50] if we don't know what the input is. (i.e. the null terminator (assuming there is one) could be after the 50th char in the src array.
x and y are not initialized before use. This is usually bad practice?

if (x=1) is usually a programming error - depending on whether assignment (rather than straight equality) is required. In this case, it looks wrong in context.

re: Spot the Bug - August 14, 2005

Ryan Russell — Mon, 15 Aug 2005 10:13:00 GMT

Well, the point is bug density, right?

char dest[50], src[100];
int x, y;//should probably be more specifc about what kind of int we want

if (x=1)//assignment, you probably meant comparison. If you meant comparison, x is uninitialized.
{
strcpy(dest,src);//could overflow. We don't see src filled in here, so it contains random stuff, unless that was just left out of the example.
dest[50] = '\0';//the last array slot is dest[49]
}

return y;//And what is y supposed to be? It's uninitialzied

re: Spot the Bug - August 14, 2005

Ceboz — Mon, 15 Aug 2005 10:35:21 GMT

Another one for you...
Assuming that X != 1 when running the above snippet and a modification is made to say "if (x==1)" as per the above comments.
Let's say X is expliticitly initialized to 0 as part of a code change, then dest is not initialized (the strcpy doesn't happen when x != 0). Any code after the if{} using dest as a null terminated string may access memory outside of the char[50] range.

I recommend something like:
if(x==1)
{
strcpy(dest,src);
dest[49] = '\0';
}
else
{
// make sure dest[] is null terminated
dest[0] = '\0';
}

Any thoughts?

re: Spot the Bug - August 14, 2005

Radu Grigore — Mon, 15 Aug 2005 15:28:25 GMT

Is there anything right with this code? :p (except the syntax)

re: Spot the Bug - August 14, 2005

Dan Piton — Mon, 15 Aug 2005 19:23:40 GMT

This is a bit nonsensical, but I'll bite.

char dest[50], src[100]; //1a
int x, y; //1b

if (x=1) //2
{
strcpy(dest,src); //3
dest[50] = '\0'; //4
}

return y; //5

1a) Nothing is initialized. One would think dest should be as long as src. Perhaps hand "dest" to a ZeroMemory call.

1b) Again, nothing is initialzied and the names are weak.

2) Assignment is very likely wrong, how about "if( 1 == x )"?

3) strcpy isn't evil but it's close, perhaps consider using strncpy. If strcpy required, perhaps make "src[49] = NULL;"

4) "dest[50] = '\0';" is corrupting memory one character beyond the end of the array.

5) "y" is returned never having been initialized at all.

Additional comments:
- Even if initialized, the value y should probably change somewhere to make the retval mean something.
- If "src" is actually passed in to us, I'd like seen an "ASSERT( src )" and/or "IsBadReadPtr( src, 100 )".
- dest is allocated on the stack, can be populated, and then goes unused before the stack unwinds

re: Spot the Bug - August 14, 2005

Dan Piton — Mon, 15 Aug 2005 19:25:34 GMT

This is a bit nonsensical, but I'll bite.

char dest[50], src[100]; //1a
int x, y; //1b

if (x=1) //2
{
strcpy(dest,src); //3
dest[50] = '\0'; //4
}

return y; //5

1a) Nothing is initialized. One would think dest should be as long as src. Perhaps hand "dest" to a ZeroMemory call.

1b) Again, nothing is initialzied and the names are weak.

2) Assignment is very likely wrong, how about "if( 1 == x )"?

3) strcpy isn't evil but it's close, perhaps consider using strncpy. If strcpy required, perhaps make "src[49] = NULL;"

4) "dest[50] = '\0';" is corrupting memory one character beyond the end of the array.

5) "y" is returned never having been initialized at all.

Additional comments:
- Even if initialized, the value y should probably change somewhere to make the retval mean something.
- If "src" is actually passed in to us, I'd like seen an "ASSERT( src )" and/or "IsBadReadPtr( src, 100 )".
- dest is allocated on the stack, may be populated, and then goes unused before the stack unwinds
- Debugging Applications by John Robbins is the best programming book on the planet.

re: Spot the Bug - August 14, 2005

Bill — Tue, 16 Aug 2005 00:59:28 GMT

char dest[50], src[100];
int x, y;

if (x=1)
{
strcpy(dest,src);
dest[50] = '\0';
}

return y;

There are three bugs here. I'm assuming that this is a local function, so initialization is already done by the compiler for me.

1. x=1 is an assignment, while not wrong is probably not what you expected. Either way it falls through and executes the if.

2. Copy of a 100 byte array to a 50 byte array is bound to cause problems. Can we say buffer overflow anyone??

3. Setting dest[50] is a big no no. It hasn't been allocated by the compiler so it will corrupt the stack. This is my biggest gripe about the C/C++ language. Its non obvious to some poor smuck who is just learning the language that you have to ALWAYS count from 0, even when it hurts.

4. Although technically not a bug, perhaps an unintended feature, the return of an unassigned y is always 0. Probably should have looked at the compiler warnings about using unassigned variables.

re: Spot the Bug - August 14, 2005

Bart — Thu, 18 Aug 2005 05:02:02 GMT

Hi folks,

It's a good programming practice to change the order of comparison statements (despite the commutative properties). Let me give you the following example:

if (x == 1)

versus

if (1 == x)

The latter one maybe isn't easier to read, but it's safer against mistakes. Assume you forget one = symbol, leading to:

if (x = 1)

versus

if (1 = x)

The latter one will be caught by the C/C++ compiler as an error: you can't assign a variable to a constant. The former one can possible generate a warning but it's legal syntax, so you can overlook the problem.

Allow me to refer to a post on my blog (http://blogs.bartdesmet.net/bart/archive/2005/07/27/3172.aspx) where I'm using this guideline in Win32 API programming.

Kind regards,

Bart De Smet