Robert Seacord wrote On 07/31/07 10:28,:
[...] As a result, we would greatly appreciate your time
and expertise in reviewing the standard, and we will acknowledge your
contributions.
General observation: The standard intends to promote
"security," but a good deal of it seems to have the effect
of avoiding "bugs." That's a worthy aim, but the two goals
don't seem to me to be precisely equivalent.
General observation: The navigation on this site makes
sequential reading cumbersome: The only (apparent) way to
navigate from a leaf to the next leaf is to crawl back up
the tree to the parent first. Cannot we have Next (and maybe
Previous) links like everybody else?
General observation: Many of the "non-compliant" examples
contain code for which the compiler is obliged to issue a
diagnostic. It seems to me that this makes them less likely
to cause trouble than other, not-necessarily-diagnosed errors.
Introduction/Rules Versus Recommendations, second point
in the definition of a rule:
There is an enumerable set of exceptional conditions
(or no such conditions) in which violating the coding
practice is necessary to ensure the correct behavior
for the program.
Perhaps my reading comprehension has deteriorated, but I can
make no sense of this sentence. The first part describes a
situation in which there is a set of conditions or there are
no conditions; isn't this vacuously true? It sure looks an
awful lot like `true OR false' to me. Then the second part
speaks of a program that only works correctly when the rule
is violated; hence if the program is compliant it does not
work, and if it works it cannot be compliant. Is that what
is really intended?
PRE02-A makes `#define COUNT 42' non-compliant, which
seems extreme. It also makes
#ifdef FROBOZZ_MAGIC_C
#define FAR far
#else
#define FAR /* nil */
#endif
non-compliant. Isn't this rather draconian?
PRE03-A just seems confused: Anybody who defines his own
overriding macro for a Standard library function deserves a
good scolding, but for treading on the library's name space
rather than for defining a macro. And why is it a generally
good idea to avoid the macros that the library has lovingly
provided for you? Do you *like* making an actual function
call for every memcpy() or isdigit() or getc() or sqrt()?
And why is avoiding such a macro "likely to increase system
security" (one of the requirements for a recommendation)?
DCL00-A seems highly debatable. Also, the second point
in the list of contexts where const objects cannot be used
is not quite right as of C99. (Maybe there'll be a rule later
on to avoid using this particular C99 feature; it strikes me
as something that makes an existing risk much worse.)
DCL01-A, first bullet: Not even in a scope where the
global variable is unknown? Why not?
DCL02-A seems to render PRE30-C moot.
DCL03-A seems misguided. In the example, the root of
the problem isn't the placement of the qualifier, but the
use of a typedef for a data pointer type. (Maybe there'll
be a rule against that later on -- but since the typedef is
still there in the "compliant solution," maybe not. There
ought to be such a rule, IMHO.)
DCL04-A: Oh, fiddlesticks. I spit upon your silly
rule; my qsort() comparators almost always begin with
int compare(const void *pp, const void *qq) {
const Type *p = pp, *q = qq;
and I intend to go right on doing so. Layout is not an
appropriate subject matter for rule-making unless a positive
benefit can be found; restricting the programmer's freedom
of layout also restricts his ability to *use* layout to make
the code more readable. The example given is such a tired
old newbie mistake that it's hardly worth mentioning -- if
you're using programmers who could make it, no amount of
rules and standards are going to make your system secure
anyhow.
DCL05-A shows one of the more famously difficult-to-
eyeball declarations, but since the programmer never needs
to write the declaration anyhow (and never needs to read it
unless he's perusing the implementation's headers, in which
case he'll run into far more baffling things), it's hardly
helpful in deciding when to use the "clarifying" typedef.
Is `int (*func)(int)' too complex? If not, why not? Where's
the dividing line?
DCL06-A is often a good practice, but can be taken too
far. Surely, exceptions can be made for 0 and 1, and maybe
for 2. Do you *really* think
enum { TWO = 2 }; /* a scalar */
enum { FOUR = 4 }; /* a scalar */
enum { SQUARE = 2 }; /* an exponent */
x = (-b + sqrt(pow(b,SQUARE) - FOUR*a*c))
/ (TWO * a);
is an improvement on
x = (-b + sqrt(b*b - 4*a*c)) / (2*a);
? Also, why is the "compliant solution" compliant? The
magic number 18 still appears in it, albeit in a different
context.
DCL07-A needs an editing pass. Also, I do not see the
claimed undefined behavior in the non-compliant example, and
the comments on the static variables in both code samples
are wrong (the identifier has no linkage, not internal linkage).
DCL08-A: Note that the compiler must issue a diagnostic
for the code in the non-compliant example. Also, the calling
mechanism described in the risk assessment is by no means
universal (the machine in front of me at this moment doesn't
use it). (By the way, why is this in the DCL section?)
DCL09-A needs a note to the effect that it relies on a
type not defined in any Standard header (indeed, a type that
Standard headers are forbidden to declare).
DCL20-A: Why the distinction between "must" call va_start
but "should" call va_end? "Must" and "must" would be better.
(And why is this in DCL?)
DCL11-A: Again, too much (non-portable) detail about the
mechanism of the failure. There is no assurance that "the
integer is silently converted into a pointer;" what actually
happens may be completely different. The code is in error,
but does not necessarily fail in the manner described. Similar
remarks pertain to the second example. (Why is it in DCL?)
DCL30-C: I don't think this should be a "rule," because
as far as I know there is no way to verify conformance in
all cases (third requirement in definition of "rule"). Or
can you propose a verification method? (And why is this
in DCL?)
DCL31-C: The advice is good, but the first example really
goes to a different point: Don't write free-hand declarations
of Standard library functions (indeed, of your own library
functions, except in the library's own headers). Also, note
that a C99 compiler must issue diagnostics for the code in
the third and fourth examples, and the "implicitly defined"
remark after the fourth example is incorrect for C99.
DCL32-C overstates its case: Why should (for example)
no-linkage identifiers in different scopes be unique?
DCL34-C: The "compliant solution" may comply to CERT's
standard, but it fails to comply to C's standard. (Hint:
What is the type sig_atomic_t for?) Also, why is the "risk
assessment" talking about a completely different qualifier?
DCL35-C forbids a common and useful practice. Also,
note that a diagnostic is required for the code in the non-
compliant example.
INT00-A might make mention of <limits.h>.
INT01-A offers two "compliant solutions" that call malloc()
and then use the returned value WITHOUT CHECKING FOR NULL!!!
Go to Jail. Go directly to Jail. Do not pass Go. Do not
collect $200. Also, why does the second compliant solution
use < instead of <=? Finally, there's entirely too much
foreknowledge of the characteristics of C's data types; who
says an unsigned int is 32 bits?
INT02-A: "the sum is truncated" is a common outcome, but
not one that it guaranteed by the C language.
INT05-A should note that the compliant solution is not
a drop-in replacement for the non-compliant original. Also,
there's a bug in the solution that a quick bench-test will
easily reveal.
INT06-A ought to mention that the third argument to
strtol() should almost always be 10; I'd even delete the
"almost" if input is coming from a human being. Only a
user of C or a C derivative expects a leading zero to be
significant; ISTR you can find some reports on comp.risks
of unhealthy consequences ensuing from non-programmers
inadvertently entering base-eight values.
INT07-A: The compliant solution (!) requires a compiler
diagnostic (on the assumption that the non-compliant example
did not and that bash_input.location.string is not of type
void*). A better solution, IMHO, is to leave the pointer
type alone and to use `c = (unsigned char)*string++' to
pluck the value out.
INT08-A: In light of INT03-A and INT04-A, why do we
need this? If there's something new, I've missed it.
INT09-A: In the third compliant solution, `indigo=yellow'
and `violet=green' would be better still. (The example
suggests a kind of visual impairment; perhaps it could be
re-worked into something where the duplicated values make
more sense? Perhaps `cerise=red' and `lime=green' would
startle the reader a bit less.)
INT10-A looks like a rehash of DCL00-A; why repeat it?
The only new thing seems to be to hang "non-confirming" on
implementations that don't prevent alteration of const-
qualified objects; the label is unwarranted and incorrect.
INT11-A: Why repeat INT02-A? (Well, it's not really
a repetition: The "compliant solution" for this one has
undefined behavior if SCHAR_MAX < 255, which describes
a very large fraction of implementations.)
INT12-A: I haven't yet decided whether the main argument
of this section is right or wrong. However, the expression
`(bits.a << 24)' exhibits undefined behavior if ints aren't
at least 33 bits wide.
That's all I've got time for now (the clumsy navigation
slows me down). My general impression is that there's some
good stuff in here, but it's interlarded with oversights
major and minor, and with entirely too much specificity
about how implementations work "under the hood." If the
document wants to talk about C, let it talk about C and
not about specific implementations of C (except in the
few cases where that's unavoidable). Otherwise, change
the title to "Secure Coding for Frobozz Magic C."
.