Skip site navigation (1)Skip section navigation (2)
Date:      Thu, 24 Apr 2014 11:58:31 +0100
From:      Ben Laurie <benl@freebsd.org>
To:        Charles Swiger <cswiger@mac.com>
Cc:        "freebsd-security@freebsd.org security" <freebsd-security@freebsd.org>, Erik Cederstrand <erik+lists@cederstrand.dk>
Subject:   Re: OpenSSL static analysis, was: De Raadt + FBSD + OpenSSH + hole?
Message-ID:  <CAG5KPzxeupwCTK7-7oA1nhM7Q=Ggv-QCwBrNchM1wM3Hwvtv_w@mail.gmail.com>
In-Reply-To: <B4A7F879-588B-4414-B416-601066C4E61A@mac.com>
References:  <10999.1398215531@server1.tristatelogic.com> <50CA7E78-BB5E-4872-A272-B7374627EC12@cederstrand.dk> <B4A7F879-588B-4414-B416-601066C4E61A@mac.com>

next in thread | previous in thread | raw e-mail | index | archive | help
On 23 April 2014 20:14, Charles Swiger <cswiger@mac.com> wrote:
> Hi--
>
> On Apr 23, 2014, at 3:06 AM, Erik Cederstrand <erik+lists@cederstrand.dk>=
 wrote:
>> Den 23/04/2014 kl. 03.12 skrev Ronald F. Guilmette <rfg@tristatelogic.co=
m>:
> [ ... ]
>>> I do imagine that the truth or falsehood of your assertion may depend
>>> quite substantally on what one does or does not consider a "false
>>> positive" in this context.
>>
>> Have a look at the ~10.000 reports at http://scan.freebsd.your.org/freeb=
sd-head/ (unavailable ATM). Silly things are reported like missing return a=
t the end of main() or not free()ing memory two lines before program exit. =
There are nonsensical reports because the analyzer doesn't detect exit() in=
 a usage() function because usage() is defined in a separate compilation un=
it, or this:
>
> Sure, static analysis isn't perfect and runs into false positives, some o=
f which are truly harmless and some of which actually do indicate an area w=
here refactoring the code in light of the warning would be an improvement.
>
> It's worth noting that even if you believe that (e.g.) the clang static a=
nalyzer isn't properly doing liveness analysis and misjudging whether there=
's a dead assignment (writing to a variable which is never read), the clang=
 compiler will be using the same analysis when doing dead-code elimination =
and common-subexpression elimination and such while optimizing.

I think this is not true. I could be wrong, but I've actually worked
on clang static analysis and I think it is an entirely separate
system. Certainly there's no guarantee that a static analysis result
will be reflected in the output of the compiler.

>> int foo(int y, int z) {
>>   int x;
>>   if (y =3D=3D z) {
>>       x =3D 0;
>>   } else  {
>>       if (y !=3D z) {
>>           x =3D 1;
>>       }
>>   }
>>   return x;
>> }
>>
>> warning that x may be uninitialized. Fixing these require considerable e=
ffort 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.
>
> Ah, that's a classic example.  If you declared y and z as const, then I'd=
 agree that the compiler should be free to make assumptions that one of the=
 two if statements must be true.
>
> On the other hand, if you assume that the arguments are volatile and that=
 maybe another thread might update y or z on the stack between the time whe=
n the first if test is evaluated and the second if, one realizes that the s=
tatic analyzer might actually have a point.  (Or you're on an SMP system an=
d don't get sequential consistency / total-store ordering without memory ba=
rriers....)
>
> Sure, your code might never intentionally try to mess with the stack, but=
 there's a long history of bugs like typing ~1030 characters at a password =
prompt and blowing past a char passwd[1024] buffer that someone assumed wou=
ld be more than enough.
>
> The most straightforward changes to this snippet would be either:
>
> int foo(int y, int z) {
>   int x;
>   if (y =3D=3D z) {
>       x =3D 0;
>   } else {
>       x =3D 1;
>   }
>   return x;
> }
>
> ...or:
>
> int foo(int y, int z) {
>   int x =3D 0;
>   if (y !=3D z) {
>       x =3D 1;
>   }
>   return x;
> }
>
> Not only are both of these shorter and they pass clang's static analyzer =
without a warning, I'd argue that the second version is noticeably cleaner.
>
> Regards,
> --
> -Chuck
>
> _______________________________________________
> freebsd-security@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-security
> To unsubscribe, send any mail to "freebsd-security-unsubscribe@freebsd.or=
g"



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAG5KPzxeupwCTK7-7oA1nhM7Q=Ggv-QCwBrNchM1wM3Hwvtv_w>