Clang 3.3 Analyzer Results

Hi,

On 11 Listopad 2013, 7:33, Jeffrey Walton wrote:

I've been tasked with a quick acceptance check of Postgres for an
upcoming project. It's a quick check, so its limited to Clang's
analyzer and sanitizers.

The analyzer is reporting some findings, and some of the findings look
legitimate.

For example, it looks like there's a double `free` occurring in
streamutil.c (around line 115). Here's a screen capture of it under
scan-view: http://postimg.org/image/3ph4hkyav/. From the capture, it
looks like `password` should be set to NULL after `free` because Clang
found a path to get back to the top of the loop (which will free
`password` again`).

Probably. From a quick glance at streamutil.c, it seems to have other
issues too, not just the double free. For example it does a free on the
password, but then uses the value for dbpassword (not sure if that code
path actually is possible - maybe it always gets into the branch with
password prompt).

There's some others of interest, too. For example, Divide by Zero and
Buffer Overflows. Here's the index.html from the scan-view report:
http://postimg.org/image/tn2ovjout/.

The scan-view tar ball is a 5.5 megabytes in size (its HTML based with
a lot of mouse over markup to help understand flows), and I'm not sure
the bug reporter will take it. Plus the developers may not want it
added to the bug reporter.

Would someone know the best way to get this to the right folks?

Thanks in advance. (And sorry reporting to pgsql-general - the
developer list states emails must go elsewhere first).

IMHO pgsql-hackers is the right audience for reports like this. The 'must
ask somewhere else first' is meant for regular questions that are not that
closely related to postgresql development, and are likely to be answered
in the generic mailing lists.

Please, upload the HTML report somewhere and post a link. If it's easier
to the clang analysis, maybe post instructions on how to do that.

regards
Tomas

--
Sent via pgsql-general mailing list (pgsql-general@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-general

On Mon, Nov 11, 2013 at 2:00 AM, Tomas Vondra <tv@fuzzy.cz> wrote:

Would someone know the best way to get this to the right folks?

Thanks in advance. (And sorry reporting to pgsql-general - the
developer list states emails must go elsewhere first).

IMHO pgsql-hackers is the right audience for reports like this. The 'must
ask somewhere else first' is meant for regular questions that are not that
closely related to postgresql development, and are likely to be answered
in the generic mailing lists.

Please, upload the HTML report somewhere and post a link. If it's easier
to the clang analysis, maybe post instructions on how to do that.

The instructions are kind of easy when they are given as a recipe.
They get a little more involved when they steps have to be explained.

The recipes are below and include building Clang 3.3 from sources. The
Clang recipe lacks a couple of copies - for example,
`asan_symbolize.py` needs to be copied into $PREFIX because `make
install` does not do it. Keep the build directory around until you
have everything you need.

The Analyzer is invoked with scan-build. Its used when compiling the
package because it performs static analysis.

The Santizers are invoked with the runtime flags. They are used with
the `check` program because they perform dynamic analysis. The more
self test the better.

Jeff

##############
# Clang 3.3

# Run from a scratch directory, delete when finished

wget http://llvm.org/releases/3.3/llvm-3.3.src.tar.gz
wget http://llvm.org/releases/3.3/cfe-3.3.src.tar.gz
wget http://llvm.org/releases/3.3/compiler-rt-3.3.src.tar.gz
# wget http://llvm.org/releases/3.3/lldb-3.3.src.tar.gz
tar xvf llvm-3.3.src.tar.gz
cd llvm-3.3.src/tools
tar xvf ../../cfe-3.3.src.tar.gz
mv cfe-3.3.src clang
# tar xvf ../../lldb-3.3.src.tar.gz
# mv lldb-3.3.src/ lldb
cd ..
cd projects
tar xvf ../../compiler-rt-3.3.src.tar.gz
mv compiler-rt-3.3.src/ compiler-rt
cd ..
./configure --enable-optimized --prefix=/usr/local
make -j4
sudo make install

# Install does not install scan-build and scan-view
# Perform the copy, and/or put them on-path
sudo mkdir /usr/local/bin/scan-build
sudo cp -r tools/clang/tools/scan-build /usr/local/bin
sudo mkdir /usr/local/bin/scan-view
sudo cp -r tools/clang/tools/scan-view /usr/local/bin

##############
# Scan-view

make distclean

export CC="/usr/local/bin/clang"
export CXX="/usr/local/bin/clang++"

/usr/local/bin/scan-build/scan-build
--use-analyzer=/usr/local/bin/clang ./configure

/usr/local/bin/scan-build/scan-build --use-analyzer=/usr/local/bin/clang make

##############
# Sanitizers

make distclean

export DYLD_FALLBACK_LIBRARY_PATH=/usr/local/lib/clang/3.3/lib/darwin/
export CC=/usr/local/bin/clang
export CXX=/usr/local/bin/clang++
export CFLAGS="-g3 -fsanitize=address -fsanitize=undefined"
export CXXFLAGS="-g3 -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr"

./configure

make

make check 2>&1 | asan_symbolize.py

--
Sent via pgsql-general mailing list (pgsql-general@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-general

[moving the discussion to pgsql-hackers]

Jeffrey Walton <noloader@gmail.com> wrote:

The Analyzer is invoked with scan-build. Its used when compiling
the package because it performs static analysis.

The Santizers are invoked with the runtime flags. They are used
with the `check` program because they perform dynamic analysis.
The more self test the better.

Thanks for the recipes!

##############
# Scan-view

make distclean

export CC="/usr/local/bin/clang"
export CXX="/usr/local/bin/clang++"

/usr/local/bin/scan-build/scan-build --use-analyzer=/usr/local/bin/clang ./configure

/usr/local/bin/scan-build/scan-build --use-analyzer=/usr/local/bin/clang make

I'm currently capturing a text version of all the warnings from
this.  Will gzip and post when it finishes.  It's generating a lot
of warnings; I have no idea how many are PostgreSQL problems and
how many are false positives; will just post the whole set FWIW.  I
am using the 3.4 development nightly snapshot with these commands:

scan-build --use-analyzer=/usr/bin/clang ./configure --silent --prefix=$PWD/Debug --enable-debug --enable-cassert --enable-depend --with-libxml --with-libxslt --with-openssl --with-perl --with-python
scan-build --use-analyzer=/usr/bin/clang make -s world

##############
# Sanitizers

make distclean

export DYLD_FALLBACK_LIBRARY_PATH=/usr/local/lib/clang/3.3/lib/darwin/
export CC=/usr/local/bin/clang
export CXX=/usr/local/bin/clang++
export CFLAGS="-g3 -fsanitize=address -fsanitize=undefined"
export CXXFLAGS="-g3 -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr"

./configure

make

make check 2>&1 | asan_symbolize.py

I haven't tried this yet, but will have a go at it after I capture
the other.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-general mailing list (pgsql-general@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-general

On Mon, Nov 11, 2013 at 2:18 PM, Kevin Grittner <kgrittn@ymail.com> wrote:

I'm currently capturing a text version of all the warnings from
this. Will gzip and post when it finishes. It's generating a lot
of warnings; I have no idea how many are PostgreSQL problems and
how many are false positives; will just post the whole set FWIW. I
am using the 3.4 development nightly snapshot with these commands:

When I tried out scan-build a while ago, the results were kind of
disappointing - there were lots of false positives. Clearly the tool
was inferior to Coverity at that time. I'd be interested to see if
there has been much improvement since.

One thing I noticed at the time was that the tool didn't have any
gumption about elog() and control flow, even though IIRC at that time
we had the abort() trick (see commit
71450d7fd6c7cf7b3e38ac56e363bff6a681973c). I seem to also recall
Coverity correctly handling that, although perhaps I'm unfairly
crediting them with taking advantage of the abort() trick because of
the state of Postgres when I tried each of those two tools - it might
be that scan-build *would* have taken advantage of that at the time,
if only the trick was there.

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Nov 11, 2013 at 5:29 PM, Peter Geoghegan <pg@heroku.com> wrote:

On Mon, Nov 11, 2013 at 2:18 PM, Kevin Grittner <kgrittn@ymail.com> wrote:

I'm currently capturing a text version of all the warnings from
this. Will gzip and post when it finishes. It's generating a lot
of warnings; I have no idea how many are PostgreSQL problems and
how many are false positives; will just post the whole set FWIW. I
am using the 3.4 development nightly snapshot with these commands:

When I tried out scan-build a while ago, the results were kind of
disappointing - there were lots of false positives. Clearly the tool
was inferior to Coverity at that time. I'd be interested to see if
there has been much improvement since.

I think you are right. Coverity is a very nice tool, and Clang has
some growing to do. For example, the Clang analyzer does not
[currently] do inter-translation unit analysis. So the following will
cause a false alarm:

// test-1.c
int n;
IntializeN(&n);
DoSomethingWithN(n);

// test-2.c
IntializeN(int* n) { if(n) {*n = 5;} }

On the other hand, its easy to accommodate the analyzer because (1)
programmers are smart, and (2) analyzers are dumb. So the following
would be an easy work around to reduce the noise:

int n = 0;
IntializeN(&n);

If the assignment is extraneous, then the optimizer will remove it and
there's no performance penalty. So its no big deal and it cuts down on
the time wasted on the false positives.

Otherwise, you get into a scenario where the tool is not used. That's
a shame since we know some of its findings are legitimate.

In the end, I don't think its wise to throw the baby out with the bath
water. Learn to work with the tools, becuase the code and users will
benefit.

Jeff

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Nov 11, 2013 at 2:45 PM, Jeffrey Walton <noloader@gmail.com> wrote:

I think you are right. Coverity is a very nice tool, and Clang has
some growing to do.

To be fair to the LLVM/Clang guys, it's not as if static analysis is a
very high priority for them.

--
Peter Geoghegan

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Nov 11, 2013 at 5:18 PM, Kevin Grittner <kgrittn@ymail.com> wrote:

[moving the discussion to pgsql-hackers]

Jeffrey Walton <noloader@gmail.com> wrote:

...
##############
# Sanitizers

make distclean

export DYLD_FALLBACK_LIBRARY_PATH=/usr/local/lib/clang/3.3/lib/darwin/
export CC=/usr/local/bin/clang
export CXX=/usr/local/bin/clang++
export CFLAGS="-g3 -fsanitize=address -fsanitize=undefined"
export CXXFLAGS="-g3 -fsanitize=address -fsanitize=undefined -fno-sanitize=vptr"

./configure

make

make check 2>&1 | asan_symbolize.py

I haven't tried this yet, but will have a go at it after I capture
the other.

Be sure asan_symbolize.py is on path. Otherwise, your dumps won't have
any symbols.

Jeff

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, Nov 11, 2013 at 5:51 PM, Peter Geoghegan <pg@heroku.com> wrote:

On Mon, Nov 11, 2013 at 2:45 PM, Jeffrey Walton <noloader@gmail.com> wrote:

I think you are right. Coverity is a very nice tool, and Clang has
some growing to do.

To be fair to the LLVM/Clang guys, it's not as if static analysis is a
very high priority for them.

Absolutely. I'm very impressed with the tool (especially the dynamic
checkers). And you can't beat the price.

I'd be happy to buy every one of LLVM/Clang devs a beer :)

Jeff

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Peter Geoghegan <pg@heroku.com> wrote:

Kevin Grittner <kgrittn@ymail.com> wrote:

I'm currently capturing a text version of all the warnings from
this.  Will gzip and post when it finishes.  It's generating a lot
of warnings; I have no idea how many are PostgreSQL problems and
how many are false positives; will just post the whole set FWIW.  I
am using the 3.4 development nightly snapshot with these commands:

When I tried out scan-build a while ago, the results were kind of
disappointing - there were lots of false positives. Clearly the tool
was inferior to Coverity at that time. I'd be interested to see if
there has been much improvement since.

Perhaps it will be of some value in terms of filing additional bug
reports with clang if it proves to have so many false positives
that it has little value in evaluating PostgreSQL code.

It does seem hard to believe that clang tools would find as enough
problems that were missed by Coverity and Valgrind to account for
all the warnings that are scrolling by; but it looks like it has
pointed out at least *one* problem that's worth fixing.

Ah, it finished.  Results attached; I haven't had time to review
them yet.

--
Kevin GrittnerEDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, Nov 11, 2013 at 6:01 PM, Kevin Grittner <kgrittn@ymail.com> wrote:

Peter Geoghegan <pg@heroku.com> wrote:

Kevin Grittner <kgrittn@ymail.com> wrote:

I'm currently capturing a text version of all the warnings from
this. Will gzip and post when it finishes. It's generating a lot
of warnings; I have no idea how many are PostgreSQL problems and
how many are false positives; will just post the whole set FWIW. I
am using the 3.4 development nightly snapshot with these commands:

When I tried out scan-build a while ago, the results were kind of
disappointing - there were lots of false positives. Clearly the tool
was inferior to Coverity at that time. I'd be interested to see if
there has been much improvement since.

Perhaps it will be of some value in terms of filing additional bug
reports with clang if it proves to have so many false positives
that it has little value in evaluating PostgreSQL code.

It does seem hard to believe that clang tools would find as enough
problems that were missed by Coverity and Valgrind to account for
all the warnings that are scrolling by; but it looks like it has
pointed out at least *one* problem that's worth fixing.

Ah, it finished. Results attached; I haven't had time to review
them yet.

The sanitizers are *bad ass*, especially the undefined behavior
checker (UBC or UBSan). There are no false positives when using it.

UBSan was based upon Peng and Regher's Integer Overflow Checker (we
used IOC before Clang 3.3 because Clang prior did not have checkers).
See http://embed.cs.utah.edu/ioc/ for details.

I've used the checkers to find a number of issues in libraries during
acceptance/integration testing, including Botan, Crypto++, libevent,
libnetcpp, OpenSSL and Squid. Some libraries were so bad it was more
like "Rejection Testing" (those have not been named).

UBSan is the tool of choice when your stuff does not work after
compiling with Intel's ICC. ICC generates the fastest code I have
seen, and it is ruthless about dropping undefined behavior in an
effort to speed up execution.

By the way, that -fwrapv is a crutch for illegal programs. It would be
wise to fix the problems rather than masking them with -fwrapv. You
can use UBSan to help ferret out the problems that -fwrapv hides. See
Ian Lance Taylor's blog for details:
http://www.airs.com/blog/archives/120.

Jeff

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Kevin Grittner <kgrittn@ymail.com> writes:

It does seem hard to believe that clang tools would find as enough
problems that were missed by Coverity and Valgrind to account for
all the warnings that are scrolling by; but it looks like it has
pointed out at least *one* problem that's worth fixing.

Yeah, that's the thing --- quite a lot of people have looked at
Postgres with Coverity already. If Clang is throwing up lots and
lots of warnings, the odds are *very* high that most of them are
false positives. Running through such a list to see if there's
anything real isn't all that exciting a prospect.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane <tgl@sss.pgh.pa.us> wrote:

quite a lot of people have looked at Postgres with Coverity
already.  If Clang is throwing up lots and lots of warnings, the
odds are *very* high that most of them are false positives.
Running through such a list to see if there's anything real isn't
all that exciting a prospect.

Here is the summary of what was reported:

All Bugs:  313

API
  Argument with 'nonnull' attribute passed null:  13
Dead store
  Dead assignment:  65
  Dead increment:  11
Logic error
  Assigned value is garbage or undefined:  19
  Branch condition evaluates to a garbage value:  2
  Dereference of null pointer:  98
  Division by zero:  15
  Out-of-bound array access:  1
  Result of operation is garbage or undefined:  9
  Stack address stored into global variable:  1
  Uninitialized argument value:  74
Memory Error
  Double free:  1
  Memory leak:  1
Unix API
  Allocator sizeof operand mismatch:  3

Does anything stand out as something that is particularly worth
looking into?  Does anything here seem worth assuming is completely
bogus because of the Coverity and Valgrind passes?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Kevin Grittner <kgrittn@ymail.com> wrote:

Logic error

   Stack address stored into global variable:  1

I took a look at this one, and it is a totally legitimate use, the
reason for which is explained with this comment:

/*
 * check_stack_depth: check for excessively deep recursion
 *
 * This should be called someplace in any recursive routine that might possibly
 * recurse deep enough to overflow the stack.  Most Unixen treat stack
 * overflow as an unrecoverable SIGSEGV, so we want to error out ourselves
 * before hitting the hardware limit.
 */

Which raises the question: do these clang tools have any way to
record which "errors" have been established to be false positives,
so that they don't show up in subsequent runs.  I know Valgrind has
that.  Without such a capability, these tools don't seem very
valuable.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Peter Geoghegan (pg@heroku.com) wrote:

I seem to also recall
Coverity correctly handling that, although perhaps I'm unfairly
crediting them with taking advantage of the abort() trick because of
the state of Postgres when I tried each of those two tools - it might
be that scan-build *would* have taken advantage of that at the time,
if only the trick was there.

With Coverity, we completely 'punt' on it because we make elog(FATAL)
into a 'program-exit' case when it clearly isn't. I've discussed this
w/ the Coverity folks but, aiui, there's no solution to this any time
soon..

Thanks,

Stephen

Kevin Grittner <kgrittn@ymail.com> writes:

Does anything stand out as something that is particularly worth
looking into?� Does anything here seem worth assuming is completely
bogus because of the Coverity and Valgrind passes?

I thought most of it was obvious junk: if there were actually
uninitialized-variable bugs in the bison grammar, for instance, not only
we but half the programs on the planet would be coredumping all the time.
Not to mention that valgrind testing would certainly have caught it.

I'd suggest looking only at the reports that pertain to seldom-exercised
code paths, as those would be the places where actual bugs might possibly
have escaped notice.

One thought for the Clang people is that most of the reports such as "null
pointer dereference" presumably mean "I think I see an execution path
whereby we could get here with a null pointer". If so, it'd be awfully
helpful if the complaint included some description of what that path is.
I think Coverity does that, or at least I've seen output from some tool
that does it.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:

I think Coverity does that, or at least I've seen output from some tool
that does it.

Coverity does provide the path (including going through multiple
interations of a loop, if applicable). Doesn't make it perfect, sadly,
but I've been trying to feed back false positives to their dev group to
address. Frustratingly, it doesn't handle global variables terribly
well and I've found a couple of false positives around cases involving
them.

Thanks,

Stephen

Kevin Grittner <kgrittn@ymail.com> wrote:

Memory Error
   Double free:  1
   Memory leak:  1

These both seemed legitimate to me.  Patch attached.  Any
objections to applying it?  I realize the memory leak is a tiny one
in the regression testing code, so it could never amount to enough
to matter; but it seems worth fixing just to avoid noise in code
analyzers.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Kevin Grittner <kgrittn@ymail.com> writes:

Kevin Grittner <kgrittn@ymail.com> wrote:

Memory Error
�� Double free:� 1
�� Memory leak:� 1

These both seemed legitimate to me.� Patch attached.� Any
objections to applying it?� I realize the memory leak is a tiny one
in the regression testing code, so it could never amount to enough
to matter; but it seems worth fixing just to avoid noise in code
analyzers.

The logic, if you can call it that, in streamutil.c makes my head hurt.
I think it needs more work than what you did here --- for one thing,
the free(password) call results in values[i] becoming a dangling pointer
to freed memory, and it's not exactly obvious that that pointer will get
overwritten again before it's used.

Moreover, although the apparent intention of the dbpassword static state
is to allow a password to be saved and reused across calls, it kinda looks
like that's broken by the odd choice to make dbgetpassword also static.
At the very least that choice makes it a lot harder to analyze what will
happen in calls after the first.

I think the looping to get a password here should be thrown out and
rewritten from scratch, or at least cribbed from someplace with less
contorted logic.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

...

One thought for the Clang people is that most of the reports such as "null
pointer dereference" presumably mean "I think I see an execution path
whereby we could get here with a null pointer". If so, it'd be awfully
helpful if the complaint included some description of what that path is.
I think Coverity does that, or at least I've seen output from some tool
that does it.

Clang can be trained with asserts.

If you are certain that a parameter cannot be NULL, then pass the
knowledge onto Clang: assert(param != NULL). Clang will stop analyzing
that path for NULL-ness.

Or, you could check it for NULL and fail the function if the param is
NULL. If its a spurious test, then the optimizer will remove it.

Jeff

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers