PostgreSQL defects [LWN.net]

PostgreSQL defects

Posted Mar 9, 2006 12:40 UTC (Thu) by alvherre (guest, #18730)
Parent article: Some notes from the Coverity survey

Unfortunately, that response does not really answer the question. The possibilities would seem to be: (1) whoever paid for the "certified versions" has not fed the resulting fixes back into the mainline; (2) all of the detected defects have been introduced into the code base since the certification run was done, or (3) the tests run on the "certified versions" were less comprehensive. None of those ideas are particularly reassuring.

There's a fourth possibility: the new test runs are not nearly as polished as the paid runs were and thus have a lot of false positives.

I'm a PostgreSQL developer. It also surprised me initially that the count was so high, for there was a previous run some months ago (sponsored by EnterpriseDB). A fellow hacker, Neil Conway, was given access to the results and propagated the fixes to the open code. So I had a look at the current reports to see how could we fare so badly.

Turns out that the vast majority of the reports are probably false positives. In the PostgreSQL code, it is quite widespread to use a "bail out" function after checking for unexpected or erroneous conditions, so the code following it is never executed. However, the checker took no notice of that; I expect every single appearance of said pattern may be reported as a bug, when it's not.

Apparently, the EnterpriseDB guys had their run "configured" so that these false positives did not appear in the report. They are working on getting the configuration propagated to this new run.

Mr. Chelf is already aware of this, and the issue is being worked on. With some luck we should have a better report soon.

to post comments

PostgreSQL defects

Posted Mar 9, 2006 16:58 UTC (Thu) by madscientist (subscriber, #16861) [Link] (5 responses)

Coverity does a "deep dive" into the code, and it recognizes all standard functions that never return (abort(), exit(), exec(), etc.) So, if your "bail out" function eventually invoked one of those known functions, it would have been marked as never returning. The vast majority of such functions do in fact call a standard never-returns function ultimately (how else do you get out?) and so this isn't generally a problem.

The only ways this could be a problem are (a) your code invokes some way of bailing that isn't a recognized standard "never returns" function, or (b) the code that invokes the never returns function was not included in the Coverity database (maybe it was part of a base library that wasn't checked by Coverity. As you mention, notes can be added to Coverity to have it recognize other "never returns" functions.

I wonder if the EDG frontend groks GCC's __attribute__(()) settings... it would be nice if it did. Do you mark your functions as never returning with these?

PostgreSQL defects

Posted Mar 9, 2006 17:15 UTC (Thu) by corbet (editor, #1) [Link]

From the discussion on the postgres list, I gather that there is a "returns sometimes" function which may have confused the situation a bit.

PostgreSQL defects

Posted Mar 9, 2006 17:15 UTC (Thu) by alvherre (guest, #18730) [Link] (3 responses)

Actually our "bail out" function does a only longjmp, cleans up and continue execution somewhere else (having aborted the current transaction, etc). It would be pretty bad a database server, if it called exit() because of a problem!

Actually is slightly more complex, because the same function is invoked if you want to issue a warning or harmless notice (which continues normal execution after sending the message text to the client), a local error condition (which sends the message and longjmps), a harder error (which kills the current process) or a very critical problem (which closes all connections and restarts the database server). (These correspong to ereport(NOTICE), ereport(ERROR), ereport(FATAL) and ereport(PANIC), respectively.)

The fix that's currently being discussed involves using some #ifdef that would only be activated if the static checker tool is in use (rather than the regular compiler), which would conditionally call exit() at the end of emitting the error message. The static checker can easily detect this and act accordingly. See http://archives.postgresql.org/pgsql-hackers/2006-03/msg0...

We don't use __attribute__(()). Not sure if it would be useful with our setup.

PostgreSQL defects

Posted Mar 9, 2006 17:22 UTC (Thu) by nix (subscriber, #2304) [Link] (2 responses)

Well, you certainly could use __attribute__((noreturn)) in this situation: it doesn't mean `function never returns anywhere'; just `function never returns to its caller', so functions that always longjmp() are candidates.

It's really easy to make __attribute__'s invisible to non-GCC compilers:

#ifndef __GNUC__
#define __attribute__(x)
#endif

__attribute__((noreturn)) at least has been supported for donkey's years, so you don't have to worry about versions of GCC that support __attribute__ but not noreturn.

PostgreSQL defects

Posted Mar 9, 2006 20:05 UTC (Thu) by kleptog (subscriber, #1183) [Link] (1 responses)

You miss the point. You can't mark the function "noreturn" because it's a *sometimes return* function, depending on the arguments. If it's an ERROR or greater, it doesn't return, less it does return.

Actually, even this isn't quite true. Under some situations WARNINGs don't return either, but that's not relevent to the static checking under discussion.

The reason Coverity misses it is probably because the function ereport() is not just a function but a macro which expands to something like:

push_new_error_on_error_stack(error_level);
set_optional_error_values();
act_on_errors();

See how the error level is passed to the first function but the third function is the one that doesn't return depending on the error level. It would take a pretty clever static checker to pick this up. The proposed solution is to add an explicit:

if( error_level >= ERROR ) exit(0);

at the end of the macro. It will never get executed but it helps the static checker out.

PostgreSQL defects

Posted Mar 12, 2006 1:02 UTC (Sun) by nix (subscriber, #2304) [Link]

Ah, yes, agreed; that makes a lot of sense.

PostgreSQL defects

Posted Mar 9, 2006 17:12 UTC (Thu) by bkw1a (guest, #4101) [Link]

Another possibility is that coverity has improved its own code since
it tested the "certified" versions. It would be interesting to
re-run the tests on the certified versions, using the current coverity
code.