Secure Coding mailing list archives

Re: New Microsoft Security Tool for developers


From: der Mouse <mouse () Rodents Montreal QC CA>
Date: Sat, 13 Dec 2003 15:47:16 +0000

["David Crocker" <[EMAIL PROTECTED]>, writing about noOverflow]

Ok, here's what I think is wrong with it:

1. It's written in C.  [...]  The trouble is, there is a shortage of
good programming languages.  [...]

C is the worst one available except for all the others?  I don't think
I disagree.  Original C was fairly good for its intended use, but it's
long since gone well beyond that, in many ways.

2. Whoever wrote it doesn't know what "const" is for.  The parameter
should be "const char *str".

Maybe.  Since we can't see the interface spec, it may well be that
noOverflow is supposed to mutate the memory pointed to by its argument
(though presumably not in the snippet quoted).

4. Being a pedant, I would replace the 2 instances of
"sizeof(buffer)" to (sizeof(buffer)/sizeof(char))". That way, in 5
years time when someone decides to support Unicode and replace "char"
with "wchar_t", there is at least a chance that the conversion will
be done correctly.

There'd be a much better chance if you used
(sizeof(buffer)/sizeof(buffer[0])); that way you don't have to notice
that buffer's element's type is repeated twice in the code when
changing that type.

5. As an alternative to (4) (and better), I would define "const int
buffersize = 10;" at the beginning, replace "buffer[10]" by
"buffer[buffersize]", and replace both instances of "sizeof(buffer)"
by "buffersize".

If you're writing in C, you need to make that a #define instead.  What
you describe might be legal C++ (where I understand a const int counts
as a compile-time constant), or legal gcc (where array sizes don't have
to be compile-time constants), but I'm pretty sure it's not legal C.

6. There should be a clear comment at the start that the input string
will be truncated without warning if it exceeds 10 characters,
thereby changing the semantics from what the user probably expects.

This depends on what the user's expectation is; it may be that this
truncation is the whole point of calling noOverflow.  (Once again,
we're back to the lack of an interface spec to let us tell what's a bug
and what's a feature.)

/~\ The ASCII                           der Mouse
\ / Ribbon Campaign
 X  Against HTML               [EMAIL PROTECTED]
/ \ Email!           7D C8 61 52 5D E7 2D 39  4E F1 31 3E E8 B3 27 4B








Current thread: