Re: Clang 3.3 Analyzer Results

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`).

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).

Jeff

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

On Mon, Nov 11, 2013 at 2:00 AM, Tomas Vondra <tv(at)fuzzy(dot)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

[moving the discussion to pgsql-hackers]

Jeffrey Walton <noloader(at)gmail(dot)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

On Mon, Nov 11, 2013 at 2:18 PM, Kevin Grittner <kgrittn(at)ymail(dot)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

On Mon, Nov 11, 2013 at 5:29 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Nov 11, 2013 at 2:18 PM, Kevin Grittner <kgrittn(at)ymail(dot)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

On Mon, Nov 11, 2013 at 2:45 PM, Jeffrey Walton <noloader(at)gmail(dot)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

On Mon, Nov 11, 2013 at 5:18 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> [moving the discussion to pgsql-hackers]
>
> Jeffrey Walton <noloader(at)gmail(dot)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

On Mon, Nov 11, 2013 at 5:51 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> On Mon, Nov 11, 2013 at 2:45 PM, Jeffrey Walton <noloader(at)gmail(dot)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

Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)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(at)ymail(dot)com> wrote:
> Peter Geoghegan <pg(at)heroku(dot)com> wrote:
>> Kevin Grittner <kgrittn(at)ymail(dot)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

Kevin Grittner <kgrittn(at)ymail(dot)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

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Kevin Grittner <kgrittn(at)ymail(dot)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

* Peter Geoghegan (pg(at)heroku(dot)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(at)ymail(dot)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

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)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(at)ymail(dot)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(at)ymail(dot)com> writes:
> Kevin Grittner <kgrittn(at)ymail(dot)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

On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

On 2013-11-12 15:17:18 -0500, Jeffrey Walton wrote:
> On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.

It might not recognize our Assert() because it expands as:
#define TrapMacro(condition, errorType) \
((bool) ((! assert_enabled) || ! (condition) || \
(ExceptionalCondition(CppAsString(condition), (errorType), \
__FILE__, __LINE__), 0)))

#define Assert(condition) \
Trap(!(condition), "FailedAssertion")

Kevin, perhaps it reports less errors if you remove the assert_enabled
check from TrapMacro? I guess you already compiled with --enable-cassert?

> 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.

Only in the case it can prove that it's redundant - and in that case the
analyzer presumably wouldn't have reported the error in the first place.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 12, 2013 at 3:25 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-11-12 15:17:18 -0500, Jeffrey Walton wrote:
>> On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.
>
> It might not recognize our Assert() because it expands as:
> #define TrapMacro(condition, errorType) \
> ((bool) ((! assert_enabled) || ! (condition) || \
> (ExceptionalCondition(CppAsString(condition), (errorType), \
> __FILE__, __LINE__), 0)))
>
> #define Assert(condition) \
> Trap(!(condition), "FailedAssertion")
>
> Kevin, perhaps it reports less errors if you remove the assert_enabled
> check from TrapMacro? I guess you already compiled with --enable-cassert?
Also see http://clang-analyzer.llvm.org/annotations.html (ignore the
OS X specific stuff). There's a couple of ways to annotate source code
and custom asserts. In this case, a `noreturn` annotation will
probably do the trick.

You can even guard it under the Clang analyzer (notwithstanding the
opinions of polluting source code with #define):

#ifdef __clang_analyzer__
// Code to be analyzed or modified
#endif

>> 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.
>
> Only in the case it can prove that it's redundant - and in that case the
> analyzer presumably wouldn't have reported the error in the first place.
:)

Jeff

On 2013-11-12 15:33:13 -0500, Jeffrey Walton wrote:
> On Tue, Nov 12, 2013 at 3:25 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-11-12 15:17:18 -0500, Jeffrey Walton wrote:
> >> On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.
> >
> > It might not recognize our Assert() because it expands as:
> > #define TrapMacro(condition, errorType) \
> > ((bool) ((! assert_enabled) || ! (condition) || \
> > (ExceptionalCondition(CppAsString(condition), (errorType), \
> > __FILE__, __LINE__), 0)))
> >
> > #define Assert(condition) \
> > Trap(!(condition), "FailedAssertion")
> >
> > Kevin, perhaps it reports less errors if you remove the assert_enabled
> > check from TrapMacro? I guess you already compiled with --enable-cassert?

> Also see http://clang-analyzer.llvm.org/annotations.html (ignore the
> OS X specific stuff). There's a couple of ways to annotate source code
> and custom asserts. In this case, a `noreturn` annotation will
> probably do the trick.

ExceptionalCondition is annotated with noreturn, but that doesn't
necessarily help because the compiler cannot know that assert_enabled is
true.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 12, 2013 at 9:38 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)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.
Clang also has a page "FAQ and How to Deal with Common False
Positives," http://clang-analyzer.llvm.org/faq.html. It demonstrates
how to force analysis on a path.

Jeff

On Tue, Nov 12, 2013 at 3:35 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-11-12 15:33:13 -0500, Jeffrey Walton wrote:
>> On Tue, Nov 12, 2013 at 3:25 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > On 2013-11-12 15:17:18 -0500, Jeffrey Walton wrote:
>> > ...
>> > It might not recognize our Assert() because it expands as:
>> > #define TrapMacro(condition, errorType) \
>> > ((bool) ((! assert_enabled) || ! (condition) || \
>> > (ExceptionalCondition(CppAsString(condition), (errorType), \
>> > __FILE__, __LINE__), 0)))
>> >
>> > #define Assert(condition) \
>> > Trap(!(condition), "FailedAssertion")
>> >
>> > Kevin, perhaps it reports less errors if you remove the assert_enabled
>> > check from TrapMacro? I guess you already compiled with --enable-cassert?
>
>> Also see http://clang-analyzer.llvm.org/annotations.html (ignore the
>> OS X specific stuff). There's a couple of ways to annotate source code
>> and custom asserts. In this case, a `noreturn` annotation will
>> probably do the trick.
>
> ExceptionalCondition is annotated with noreturn, but that doesn't
> necessarily help because the compiler cannot know that assert_enabled is
> true.
Oh, my bad. I overlooked that.

Jeff

On 11/11/13, 1:33 AM, Jeffrey Walton wrote:
> The analyzer is reporting some findings, and some of the findings look
> legitimate.

We have been tracking clang scan-build results for some time, and fixed
quite a few of them. Most of the remaining ones are false positives.
Maybe there are still a few that could be fixed, but certainly
scan-build is not suitable as an acceptance test of the PostgreSQL
source at this point.

On 11/12/13, 8:18 AM, Kevin Grittner wrote:
> Here is the summary of what was reported:
>
> All Bugs: 313

> 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 have tracked scan-build for some time, and I'm sure that almost all of
these bugs are false positives at this point.

I have a private branch somewhere that I have badly hacked up (e.g.,
hardcoding enable_assert = 1), which gets that number down to 251
according to my latest notes. That's about the best you can hope for.

Btw., you can also keep score here:
http://pgci.eisentraut.org/jenkins/view/PostgreSQL/job/postgresql_master_scan-build/.
This uses an older version of clang, so the number of bugs is lower,
but the nature of the bugs is also more stupid. I plan to upgrade that
at some point.

It's worth keeping an eye on this, but it's not worth losing sleep over.

Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:

> I have tracked scan-build for some time, and I'm sure that almost
> all of these bugs are false positives at this point.

From poking around, I agree.  One particular error I noticed that
it makes a lot is that in a loop it says that an assigned value is
not referenced if the reference will not be hit until the next
iteration of the loop.

> Btw., you can also keep score here:
> http://pgci.eisentraut.org/jenkins/view/PostgreSQL/job/postgresql_master_scan-build/

Cool.  I wasn't aware that anyone was already looking at this.

> It's worth keeping an eye on this, but it's not worth losing
> sleep over.

Agreed in general; however, with this 3.4 development build the
"Memory Error" section only showed two problems, and those were the
only two problems I found that were real.  It might be worth
monitoring that one section.

If nobody objects, I'll fix that small memory leak in the
regression test driver.  Hopefully someone more familiar with
pg_basebackup will fix the double-free (and related problems
mentioned by Tom) in streamutil.c.

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

On Tue, Nov 12, 2013 at 6:04 PM, Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
>
>> I have tracked scan-build for some time, and I'm sure that almost
>> all of these bugs are false positives at this point.
>
> From poking around, I agree. One particular error I noticed that
> it makes a lot is that in a loop it says that an assigned value is
> not referenced if the reference will not be hit until the next
> iteration of the loop.
>
>> Btw., you can also keep score here:
>> http://pgci.eisentraut.org/jenkins/view/PostgreSQL/job/postgresql_master_scan-build/
>
> Cool. I wasn't aware that anyone was already looking at this.
>
>> It's worth keeping an eye on this, but it's not worth losing
>> sleep over.
>
> Agreed in general; however, with this 3.4 development build the
> "Memory Error" section only showed two problems, and those were the
> only two problems I found that were real. It might be worth
> monitoring that one section.
>
> If nobody objects, I'll fix that small memory leak in the
> regression test driver. Hopefully someone more familiar with
> pg_basebackup will fix the double-free (and related problems
> mentioned by Tom) in streamutil.c.
Well, how about the use of the unintialized values?

I did not check any with the long path lengths, but the
`pqsecure_write` in fe-secure.c looks valid to me. `spinfo` is
declared, Clang builds/finds the path, then the unitializaed `spinfo`
is used in `RESTORE_SIGPIPE(conn, spinfo);`.

Anyway, I don't mean to sound argumentative.

Jeff

Kevin Grittner escribió:

> 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.

We have marked a large number of memory leak reports by Coverity in
initdb and other short-lived programs as false positive, on the grounds
that there's no point in freeing memory in a program that's about to
terminate anyway. I'm not saying I agree necessarily with that POV, but
if we take that stance then there's similarly no point in fixing this
leak in the regression test code, is there?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Tue, Nov 12, 2013 at 5:19 PM, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On 11/12/13, 8:18 AM, Kevin Grittner wrote:
>> Here is the summary of what was reported:
>>
>> All Bugs: 313
>
>> 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 have tracked scan-build for some time, and I'm sure that almost all of
> these bugs are false positives at this point.
>
> I have a private branch somewhere that I have badly hacked up (e.g.,
> hardcoding enable_assert = 1), which gets that number down to 251
> according to my latest notes. That's about the best you can hope for.
>
> Btw., you can also keep score here:
> http://pgci.eisentraut.org/jenkins/view/PostgreSQL/job/postgresql_master_scan-build/.
> This uses an older version of clang, so the number of bugs is lower,
> but the nature of the bugs is also more stupid. I plan to upgrade that
> at some point.
I thinks its good Postgres is using the tools and publishing the results.

The reports being generated with Clang 3.3 on Postgres 9.3.1 are
different that posted. For example, french.c is not listed in the
Clang 3.3 reports. What version of Clang is used in the online report?

Jeff

On Tue, Nov 12, 2013 at 7:11 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Kevin Grittner escribió:
>
>> 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.
>
> We have marked a large number of memory leak reports by Coverity in
> initdb and other short-lived programs as false positive, on the grounds
> that there's no point in freeing memory in a program that's about to
> terminate anyway. I'm not saying I agree necessarily with that POV, but
> if we take that stance then there's similarly no point in fixing this
> leak in the regression test code, is there?
Ah, OK. So I would disagree here.

Test code has to meet the same standards as production code.
Otherwise, it could create spurious noise that could mask real
findings :)

It kind of begs a few questions. How is an user, integrator or auditor
supposed to know ....

* devs write 'real code' in the libraries and programs, as opposed to
'non-real code' in their test suite
* what the devs have deemed 'not a good expenditure of resources'?

Anyway, its just my philosophy. I know many projects share the
opposing point of view.

Jeff

Jeffrey Walton <noloader(at)gmail(dot)com> writes:
> On Tue, Nov 12, 2013 at 7:11 PM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
>> We have marked a large number of memory leak reports by Coverity in
>> initdb and other short-lived programs as false positive, on the grounds
>> that there's no point in freeing memory in a program that's about to
>> terminate anyway. I'm not saying I agree necessarily with that POV, but
>> if we take that stance then there's similarly no point in fixing this
>> leak in the regression test code, is there?

> Ah, OK. So I would disagree here.
> Test code has to meet the same standards as production code.

No, this isn't about test code vs production, it's about not bothering
to free memory explicitly when a program is about to terminate. Alvaro
is suggesting that the proposed addition to pg_regress.c is just a waste
of cycles. IMO it's not that big a deal either way in this case, since
it's just one line of code that isn't going to take too long. However,
I would push back pretty darn hard if you suggested that, say, the
server needed to explicitly free everything it'd allocated before
terminating --- the amount of bookkeeping needed for that would be
ginormous, and it would be 100% pointless.

From a testing standpoint, that means we need a way to explain to
any analysis code that we don't care about the fact that specific
allocations will still be there at termination. This isn't all that
much different from suppressing other types of false positive, of
course. As Alvaro and a couple of other people have mentioned, we've
gone to some effort to identify and mark false positives with Coverity.
I'm not convinced yet that clang's tools are mature enough to justify
putting in similar effort with it.

regards, tom lane

On Tue, 2013-11-12 at 19:11 -0500, Jeffrey Walton wrote:
> The reports being generated with Clang 3.3 on Postgres 9.3.1 are
> different that posted. For example, french.c is not listed in the
> Clang 3.3 reports. What version of Clang is used in the online report?

LLVM 3.0

Jeffrey Walton <noloader(at)gmail(dot)com> writes:
> I did not check any with the long path lengths, but the
> `pqsecure_write` in fe-secure.c looks valid to me. `spinfo` is
> declared, Clang builds/finds the path, then the unitializaed `spinfo`
> is used in `RESTORE_SIGPIPE(conn, spinfo);`.

It's junk AFAICS, though I will agree that seeing that it's junk is
probably beyond clang's powers of analysis. To make use of an
uninitialized value, we'd have to arrive at RESTORE_SIGPIPE with
SIGPIPE_MASKED false, after either not having passed through
DISABLE_SIGPIPE at all, or having passed through it with SIGPIPE_MASKED
true. The first case can be dismissed out of hand. The second case
is a bit harder, because there is a place in between that clears
sigpipe_flag and thus could possibly cause SIGPIPE_MASKED to become
false. However, we immediately jump back and re-execute DISABLE_SIGPIPE
after doing that, so there's no bug. But that control flow is ugly
enough that I'm not surprised clang can't see through it ...

Having said all that, though, I'm a bit surprised that we don't get
possibly-uninitialized-variable warnings from gcc here. In older
gcc versions the address-takings would have disabled warnings, but
I thought that that didn't discourage newer versions from whining.

regards, tom lane

Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> No, this isn't about test code vs production, it's about not bothering
> to free memory explicitly when a program is about to terminate.  Alvaro
> is suggesting that the proposed addition to pg_regress.c is just a waste
> of cycles.  IMO it's not that big a deal either way in this case, since
> it's just one line of code that isn't going to take too long.

Right.  IMV, it's easier in this case to silence the warnings for
all future static code analysis runs, by this tool or any other, by
fixing it rather than having this particular triviality resurface
to annoy anyone in the future.

Fix pushed to the master branch.

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

Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> If nobody objects, I'll fix that small memory leak in the
> regression test driver. Hopefully someone more familiar with
> pg_basebackup will fix the double-free (and related problems
> mentioned by Tom) in streamutil.c.

Here's a less convoluted (IMHO) approach to the password management logic
in streamutil.c. One thing I really didn't care for about the existing
coding is that the loop-for-password included all the rest of the
function, even though there's no intention to retry for any purpose except
collecting a password. So I moved up the bottom of the loop. For ease of
review, I've not reindented the code below the new loop bottom, but would
do so before committing.

Any objections to this version?

regards, tom lane

On Wednesday, November 13, 2013, Tom Lane wrote:

> Kevin Grittner <kgrittn(at)ymail(dot)com <javascript:;>> writes:
> > If nobody objects, I'll fix that small memory leak in the
> > regression test driver. Hopefully someone more familiar with
> > pg_basebackup will fix the double-free (and related problems
> > mentioned by Tom) in streamutil.c.
>
> Here's a less convoluted (IMHO) approach to the password management logic
> in streamutil.c. One thing I really didn't care for about the existing
> coding is that the loop-for-password included all the rest of the
> function, even though there's no intention to retry for any purpose except
> collecting a password. So I moved up the bottom of the loop. For ease of
> review, I've not reindented the code below the new loop bottom, but would
> do so before committing.
>
> Any objections to this version?
>
> Nope, looks good to me.

That code was originally "stolen" from psql, and then whacked around a
number of times. The part about looping and passwords, for example, is in
startup.c in psql as well. We probably want to fix it there as well (even
if it doesn't have the same problem, it has the same general design). Or
perhaps even put that function somewhere shared between the two?

It's also in pg_dump/pg_backup_db.c, there's a version of it in
pg_dumpall.c, etc. Which I think is a good argument for fixing them all by
sharing the code somewhere? In fact, we already have some in
script/common.c - but it's only used by the tools that are in script/.

//Magnus

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> That code was originally "stolen" from psql, and then whacked around a
> number of times. The part about looping and passwords, for example, is in
> startup.c in psql as well. We probably want to fix it there as well (even
> if it doesn't have the same problem, it has the same general design). Or
> perhaps even put that function somewhere shared between the two?

> It's also in pg_dump/pg_backup_db.c, there's a version of it in
> pg_dumpall.c, etc. Which I think is a good argument for fixing them all by
> sharing the code somewhere? In fact, we already have some in
> script/common.c - but it's only used by the tools that are in script/.

Hm, maybe, but where? It's inappropriate for libpgcommon (we don't
want that calling libpq), so I'm not real sure what to do with it.
Also it's not clear to me that all these tools would have the same
requirements for the non-password parameters for the connection request.

BTW, I realized while fooling with this that although the code looks like
it's intended to iterate till a correct password is obtained, actually it
cannot prompt more than once, because of the way PQconnectionNeedsPassword
is coded. Therefore, the double free that clang is worried about can't
really happen. It's still worth fixing IMO.

regards, tom lane

On Wed, 2013-11-13 at 12:43 -0500, Tom Lane wrote:
> Kevin Grittner <kgrittn(at)ymail(dot)com> writes:
> > If nobody objects, I'll fix that small memory leak in the
> > regression test driver. Hopefully someone more familiar with
> > pg_basebackup will fix the double-free (and related problems
> > mentioned by Tom) in streamutil.c.
>
> Here's a less convoluted (IMHO) approach to the password management logic
> in streamutil.c. One thing I really didn't care for about the existing
> coding is that the loop-for-password included all the rest of the
> function, even though there's no intention to retry for any purpose except
> collecting a password. So I moved up the bottom of the loop. For ease of
> review, I've not reindented the code below the new loop bottom, but would
> do so before committing.
>
> Any objections to this version?

As far as the clang static analyzer is concerned, this hasn't actually
helped, because now it's complaining about

Value stored to 'need_password' is never read

With the attached patch, that warning goes way, and the logic is
arguably slightly clearer, too.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> With the attached patch, that warning goes way, and the logic is
> arguably slightly clearer, too.

No objection.

regards, tom lane