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:
- Re: New Microsoft Security Tool for developers, (continued)
- Re: New Microsoft Security Tool for developers Liudvikas Bukys (Dec 12)
- Re: New Microsoft Security Tool for developers Avner Peled (Dec 12)
- Re: New Microsoft Security Tool for developers Jeremy Thibeaux (Dec 12)
- Re: New Microsoft Security Tool for developers Steve Litt (Dec 12)
- Re: New Microsoft Security Tool for developers Dana Epp (Dec 12)
- Re: New Microsoft Security Tool for developers Steve Litt (Dec 12)
- Re: New Microsoft Security Tool for developers Jack D. Unrue (Dec 12)
- Re: New Microsoft Security Tool for developers Dana Epp (Dec 13)
- Re: New Microsoft Security Tool for developers Liudvikas Bukys (Dec 12)
- Re: What's wrong with this code? Jared W. Robinson (Dec 12)
- RE: New Microsoft Security Tool for developers David Crocker (Dec 12)
- Re: New Microsoft Security Tool for developers der Mouse (Dec 13)
- Re: New Microsoft Security Tool for developers der Mouse (Dec 13)
- Re: New Microsoft Security Tool for developers der Mouse (Dec 13)
- Re: New Microsoft Security Tool for developers Mark Graff (Dec 13)
- Re: New Microsoft Security Tool for developers der Mouse (Dec 15)
- Re: New Microsoft Security Tool for developers Dana Epp (Dec 15)
