PostgreSQL defects [LWN.net]

PostgreSQL defects

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

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