Date: Wed, 23 Apr 2014 12:14:40 -0700 From: Charles Swiger <cswiger@mac.com> To: Erik Cederstrand <erik+lists@cederstrand.dk> Cc: "freebsd-security@freebsd.org security" <freebsd-security@freebsd.org> Subject: Re: OpenSSL static analysis, was: De Raadt + FBSD + OpenSSH + hole? Message-ID: <B4A7F879-588B-4414-B416-601066C4E61A@mac.com> In-Reply-To: <50CA7E78-BB5E-4872-A272-B7374627EC12@cederstrand.dk> References: <10999.1398215531@server1.tristatelogic.com> <50CA7E78-BB5E-4872-A272-B7374627EC12@cederstrand.dk>
next in thread | previous in thread | raw e-mail | index | archive | help
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.com>: [ ... ] >> 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/freebsd-head/ (unavailable ATM). Silly things are reported like missing return at 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 unit, or this: Sure, static analysis isn't perfect and runs into false positives, some of which are truly harmless and some of which actually do indicate an area where 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 analyzer 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. > 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. Fixing these require considerable effort 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 when the first if test is evaluated and the second if, one realizes that the static analyzer might actually have a point. (Or you're on an SMP system and don't get sequential consistency / total-store ordering without memory barriers....) 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 would be more than enough. The most straightforward changes to this snippet would be either: int foo(int y, int z) { int x; if (y == z) { x = 0; } else { x = 1; } return x; } ...or: int foo(int y, int z) { int x = 0; if (y != z) { x = 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?B4A7F879-588B-4414-B416-601066C4E61A>