PostgreSQL defects

Posted Mar 9, 2006 17:15 UTC (Thu) by alvherre (subscriber, #18730)
In reply to: PostgreSQL defects by madscientist
Parent article: Some notes from the Coverity survey

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.