Date: Thu, 24 Apr 2014 04:07:09 -0700 From: "Ronald F. Guilmette" <rfg@tristatelogic.com> To: "freebsd-security@freebsd.org" <freebsd-security@freebsd.org> Subject: Re: OpenSSL static analysis, was: De Raadt + FBSD + OpenSSH + hole? Message-ID: <23494.1398337629@server1.tristatelogic.com> In-Reply-To: <50CA7E78-BB5E-4872-A272-B7374627EC12@cederstrand.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <50CA7E78-BB5E-4872-A272-B7374627EC12@cederstrand.dk>,
Erik Cederstrand <erik+lists@cederstrand.dk> wrote:
>Silly things are reported like missing return at the end of main()
In the post that you are replying to, I took issue with two prior assertions
made by Mark Andrews, specifically (1) that some clang static analysis
warnings are "false positives" and (2) that elimination some of them was
"impossible".
Would you agree with me if I were to say that if in fact there is no return
statement at the end of the main() function, then a warning which simply
states that fact is not in any sense "false"? And if you would agree with
me on that, would you also and likewise agree that such a warning in exactly
such a context cannot and should not be considered, by any reasonable person
to be a "false" postive?
And secondly, continuing with this same example (i.e. of a missing return
at the end of main) would you agree with me that elimination of a warning
about that very specific and particular condition would not only be entirely
"possible", but actually entirely straightforward?
>or not free()ing memory two lines before program exit.
Again, if a clang static analysis is generating a warning message saying
that a chunk of malloc'd memory is not being free'd before program exit,
and if in fact that memory is not being free'd before program exit, then
how does such a message in any sense constitute a "false positive"? And
also, in what sense, if any, might it be either "impossible"... or even
particularly difficult for that matter... to eliminate exactly such a
warning message in exactly such a context? Wouldn't simply adding a
suitable call to free() make the warning go away? Assuming so, then what,
exactly, is the problem?
>There are nonsensical
>reports because the analyzer doesn't detect exit() in a usage() function
>because usage() is defined in a separate compilation unit,
If there does not exist, at present, any mechanism (supported by the clang
compiler) to accomodate this precise situation, then this is certainly
something that would bear fixing (in that compiler). I know that GNU C
has a special pragma (__noreturn__) which can be attached to external
function declarations _precisely_ to deal with this exact sort of situa-
tion, i.e. otherwise unhelpful and misleading results from static analysis
that does not take into account that a given external function causes
program exit:
http://gcc.gnu.org/onlinedocs/gcc-4.3.5/gcc/Function-Attributes.html
"This makes slightly better code. More importantly, it helps
avoid spurious warnings of uninitialized variables."
^^^^^^^^^^^^^^^^^^^^^^^
>int foo(int y, int z) {
> int x;
> if (y == z) {
> x = 0;
> } else {
> if (y != z) {
> x = 1;
> }
> }
> return x;
>}
>
>warning that x may be uninitialized.
Yes. That code will likely get exactly that warning from many C compilers.
>Fixing these require considerable effort
Sir, does not the following trivial and obvious single line modification
to the above code eliminate the warning? And does it not do so *without*
the need for ``considerable effort''?
int x = -1;
I thank you for providing me with the example above, and thus also this
opportunity to so perfectly illustrate my fundamental point.
My fundamental point is just this: I think that it is a real crying shame
that implementors of compilers... in particular GCC and clang... have worked
and striven and sweated so long and so hard to give developers such great
and often quite helpful compile-time warnings... warnings which, if heeded,
may often prevent serious bugs from making it out the door... only to find
that many a developer simply refuses to pay any heed at all to the warnings
because (a) not all of them indicate absolutely DIRE problems and because
(b) eliminating the ones that don't is _perceived_ by many... wrongly in
my opinion... to be ``too hard''.
As the examples above illustrate, the claim that eliminating the non-helpful
warnings is ``too hard'' cannot, I believe, be supported by the facts, and
the (unfortunately widespread) perception that eliminating such warnings is
``too hard'' is actually... and not to put too fine a point on it... a
product more of ignorance or laziness than it is a product of genuine
difficulty in quieting the unhelpful diagnostics in question.
(To me it is a little like going for a drive with your cranky eighty year
old grandfather who, even when out on the freeway, refuses to engage the
cruise control because (he claims) it is ``too hard'' to enable it.
Actually, as most of us know, it really just ain't that bloody hard.)
In short, neither the use of modern cruise control systems nor the elimination
of the kinds of static compiler generated warnings we have been discussing
is in fact very hard at all, and in both cases there are substantial benefits
to be derived from making proper and full use of these modern tools. (Those
benefits explain why the implementors of these things went to such a lot
of time and bother to actually build these things.)
>e.g. improving IPA and adding alpha-remaning support to the
>analyzer's constraint manager, or would result in unnecessary code churn
>in FreeBSD just to work around the reports.
I'm sorry, you are apparently using some domain-specific terminology with
which I am not familiar. What exactly is "IPA" and "alpha-remaning"? Do
these have something do do with SSL?
As regards to your claim relating to "unnecessary code churn", I'm not sure
what I can offer, other than the observation that at some or many times
during this month of April, 2014 I believe that there have existed many
many many system administrators, throughout the world, who, I believe,
rather seriously _do_ wish that the code of OpenSSL had been ``churned''
a bit more than it had been prior to this month. In other words I'm not
100% sure that the loaded term ``code churn'' carries quite the perjorative
connotation that you had intended. Not in the present context anyway.
If ``code churn'' causes bugs to be ``churned'' *out* of production code,
then it is a Good Thing, not a Bad Thing.
>My best guess is that at least 90% of the reports are either false
>positives or really silly.
As I've noted above, a warning that says that a function is missing a return
statement when it is in fact missing a return statement is not in any sense
a ``false positive''. It may be a TRUE positive that many programmers may,
in their haste to ship something, prefer to ignore, but that is a judgement
call which could be made in a different way by a different programmer. And
more to the point, the programmer who does in fact wish to ignore that
specific warning can trivially banish it from view, forever, simply by
adding the requsite return statement... even if, due to actual run-time
behavior, that added return statement will be essentially superfluous
(at run-time).
>Which doesn't mean that the reports are
>useless, but a lot of time is wasted finding real bugs.
I could be wrong, but I am sort-of thinking that perhaps that last part
of what you said above didn't come out quite the way you had intended.
Regards,
rfg
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?23494.1398337629>