The Design of Software (CLOSED)

A public forum for discussing the design of software, from the user interface to the code architecture. Now closed.

The "Design of Software" discussion group has been merged with the main Joel on Software discussion group.

The archives will remain online indefinitely.

It should pass static analysis, but it's still crappy code

http://weblogs.mozillazine.org/roc/archives/2006/09/static_analysis_and_scary_head.html

compNameInFile = (char *) malloc(sizeof(char) * (stbuf.st_size + 1));
...
908        memset(compNameInFile, 0 , (stbuf.st_size + 1));
...
911        rv = fread((void *) compNameInFile, sizeof(char),
912                    stbuf.st_size, dlMarkerFD);
...
923                if (strcmp(currComp->GetArchive(), compNameInFile) == 0)

This is part of the code that was marked as a vulnerability in FireFox by a static analyzer. Anyone who has tried these analyzers knows how many false positives they have, but I would say this is still crappy code.

1. It's not clear that there's not buffer overflow.
2. The malloc could leak.
3. It hits the memory allocator which is slow and it sets the buffer which is slow.
4. Usual ugly malloc casts.

C++ is a lot cleaner.
son of parnas
Thursday, September 14, 2006
 
 
Looks like the person who wrote it is not very familiar with C.

1) sizeof(char) is 1 by definition.
2) Casting the return value of malloc is always unnecessary.
3) Casting to a (void *) is redundant (unless the argument is some other pointer, which it is not in this case).
4) The entire string is memset() to 0 when only 1 character is required.
UNIX Guy
Thursday, September 14, 2006
 
 
sizeof(char) is useful for documenting intent in the code, even if it is known to compile to a no-op. I've done the same thing myself.

Ditto for casting the return value of malloc.

The memset is overkill, but it does leave a cleaner buffer while debugging. I think it's safe to assume rv <= stbuf.st_size, so I would have used:

    compNameInFile[max(0,rv)] = 0;

This almost certainly would be a perfect example of premature optimization, though.

The casting to void * is a red flag, because it totally bypasses any type checks that the compiler does. If a function really requires a foo * for example, I think the compiler would silently convert your newly cast void * pointer into one. If you explicitly cast to a foo *, it's just more good self-documenting code.
Mark Ransom Send private email
Thursday, September 14, 2006
 
 
That code is mostly correct. It looks like my old C code from 1990. ;-)

However, to be 100% foolproof, the final parameter of memset() should be identical to the parameter passed to malloc().
MBJ Send private email
Saturday, September 16, 2006
 
 
son of parnas,

"1. It's not clear that there's not buffer overflow."

There could be a comment in the snipped parts.

"2. The malloc could leak."

Any resource allocation could leak when you forget the corresponding deallocation. Any self-written allocation wrapper could leak. Any fopen() could leak a filehandle by forgetting the corresponding fclose(). Strange argument for a language missing a built-in garbage collection.

"3. It hits the memory allocator which is slow and it sets the buffer which is slow."

How do you know? Could you lend me your crystal ball to tell me the next lottery numbers?

"4. Usual ugly malloc casts."

Here I agree. The style is inconsistent and denotes a lack of knowledge of the language.

"C++ is a lot cleaner."

No, it is not. ;)
Secure
Sunday, September 17, 2006
 
 
Mark, assuming that fread() is the stdio fread(), then its first parameter IS a void*.

So, your "red flag" is in fact a perfect example of the practice you said should be used -- casting a pointer to the type the function expects.  :P
Iago
Sunday, September 17, 2006
 
 
What I was trying to say is that void * is a special case, and thus needs to be an exception to the rule.  If I cast a pointer to foo * and the function was expecting bar *, it's a compile-time error.  If I cast it to void *, it isn't.

Of course this is a bad example, because as you say fread uses void *, thus invalidating all type checking anyway.  I still contend that casting to void * is a bad habit to get into.
Mark Ransom Send private email
Tuesday, September 26, 2006
 
 

This topic is archived. No further replies will be accepted.

Other recent topics Other recent topics
 
Powered by FogBugz