Обсуждение: BUG #3841: core dump in uuid-ossp

The following bug has been logged online:

Bug reference:      3841
Logged by:          Dmitriy
Email address:      im@ionflux.ru
PostgreSQL version: 8.3.beta4
Operating system:   FreeBSD 6.2-RELEASE
Description:        core dump in uuid-ossp
Details:

in sql console:
postgres=# select uuid_nil();
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.

on server console:
LOG:  server process (PID 41258) was terminated by signal 11: Segmentation
fault
LOG:  terminating any other active server processes
WARNING:  terminating connection because of crash of another server process
DETAIL:  The postmaster has commanded this server process to roll back the
current transaction and exit, because another server pro.
HINT:  In a moment you should be able to reconnect to the database and
repeat your command.
LOG:  all server processes terminated; reinitializing
LOG:  database system was interrupted; last known up at 2007-12-27 11:53:03
IRKT
LOG:  database system was not properly shut down; automatic recovery in
progress
LOG:  redo starts at 0/43FEA4
LOG:  record with zero length at 0/447C94
LOG:  redo done at 0/447C68
LOG:  last completed transaction was at log time 2007-12-27
11:53:37.354834+08

gdb:

[postgres@dev1 ~/data]$ gdb ../bin/postgres postgres.core
GNU gdb 6.1.1 [FreeBSD]
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you
are
welcome to change it and/or distribute copies of it under certain
conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB.  Type "show warranty" for details.
This GDB was configured as "i386-marcel-freebsd"...
Core was generated by `postgres'.
Program terminated with signal 11, Segmentation fault.
Reading symbols from /usr/local/lib/libxslt.so.2...done.
Loaded symbols for /usr/local/lib/libxslt.so.2
Reading symbols from /usr/local/lib/libxml2.so.5...done.
Loaded symbols for /usr/local/lib/libxml2.so.5
Reading symbols from /usr/lib/libpam.so.3...done.
Loaded symbols for /usr/lib/libpam.so.3
Reading symbols from /lib/libcrypt.so.3...done.
Loaded symbols for /lib/libcrypt.so.3
Reading symbols from /lib/libm.so.4...done.
Loaded symbols for /lib/libm.so.4
Reading symbols from /lib/libc.so.6...done.
Loaded symbols for /lib/libc.so.6
Reading symbols from /lib/libz.so.3...done.
Loaded symbols for /lib/libz.so.3
Reading symbols from /usr/local/lib/libiconv.so.3...done.
Loaded symbols for /usr/local/lib/libiconv.so.3
Reading symbols from /usr/local/pgsql/lib/uuid-ossp.so...done.
Loaded symbols for /usr/local/pgsql/lib/uuid-ossp.so
Reading symbols from /libexec/ld-elf.so.1...done.
Loaded symbols for /libexec/ld-elf.so.1
#0  0x2a5d5cd8 in uuid_import () from /usr/local/pgsql/lib/uuid-ossp.so
(gdb) bt
#0  0x2a5d5cd8 in uuid_import () from /usr/local/pgsql/lib/uuid-ossp.so
#1  0x2a5d67d2 in uuid_load () from /usr/local/pgsql/lib/uuid-ossp.so
#2  0x2a5d57aa in special_uuid_value (name=0x2a5dafa4 "nil") at
uuid-ossp.c:95
#3  0x08141c34 in ExecMakeFunctionResult (fcache=0x1100bc8c, econtext=Cannot
access memory at address 0xe89d9715
) at execQual.c:1351
Cannot access memory at address 0xe89d970d
(gdb) Quit

Dmitriy wrote:

> in sql console:
> postgres=# select uuid_nil();
> server closed the connection unexpectedly
>     This probably means the server terminated abnormally
>     before or while processing the request.

Thanks for the report.  I think the problem here is that we're not
checking the return values of uuid functions.  For example uuid_create
could fail due to memory shortage or a number of other problems.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Dmitriy wrote:
>
> > in sql console:
> > postgres=# select uuid_nil();
> > server closed the connection unexpectedly
> >     This probably means the server terminated abnormally
> >     before or while processing the request.
>
> Thanks for the report.  I think the problem here is that we're not
> checking the return values of uuid functions.  For example uuid_create
> could fail due to memory shortage or a number of other problems.

Please try the attached patch.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Please try the attached patch.

Most of the invocations of pguuid_complain will be outright lies as to
which function is complaining.  Please rethink the error message.

            regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Please try the attached patch.
>
> Most of the invocations of pguuid_complain will be outright lies as to
> which function is complaining.  Please rethink the error message.

Doh!  Sorry.  How about this:

static void
pguuid_complain(uuid_rc_t rc)
{
    char    *err = uuid_error(rc);

    if (err != NULL)
        ereport(ERROR,
                (errmsg("OSSP uuid failure: %s", err)));
    else
        ereport(ERROR,
                (errmsg("OSSP uuid failure: error code %d", rc)));
}


Alternatively we could pass the called function name into
pguuid_complain, but I'm not sure it's worth the trouble (what does it
give the user, anyway?)

--
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre@commandprompt.com> writes:
>     if (err != NULL)
>         ereport(ERROR,
>                 (errmsg("OSSP uuid failure: %s", err)));
>     else
>         ereport(ERROR,
>                 (errmsg("OSSP uuid failure: error code %d", rc)));

Maybe "OSSP uuid library failure"?  Otherwise seems OK.

> Alternatively we could pass the called function name into
> pguuid_complain, but I'm not sure it's worth the trouble (what does it
> give the user, anyway?)

Probably not much, if the uuid_error() strings are well written.

Can we throw some more specific SQLSTATE than the default "internal
error" here?  Offhand I can't think of anything, but as a rule of
thumb an ereport() ought to have an errcode().  If it really is an
internal error then elog() is good enough.

            regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> >     if (err != NULL)
> >         ereport(ERROR,
> >                 (errmsg("OSSP uuid failure: %s", err)));
> >     else
> >         ereport(ERROR,
> >                 (errmsg("OSSP uuid failure: error code %d", rc)));
>
> Maybe "OSSP uuid library failure"?  Otherwise seems OK.

Sold.

> > Alternatively we could pass the called function name into
> > pguuid_complain, but I'm not sure it's worth the trouble (what does it
> > give the user, anyway?)
>
> Probably not much, if the uuid_error() strings are well written.

char *uuid_error(uuid_rc_t rc)
{
    char *str;

    switch (rc) {
        case UUID_RC_OK:  str = "everything ok";    break;
        case UUID_RC_ARG: str = "invalid argument"; break;
        case UUID_RC_MEM: str = "out of memory";    break;
        case UUID_RC_SYS: str = "system error";     break;
        case UUID_RC_INT: str = "internal error";   break;
        case UUID_RC_IMP: str = "not implemented";  break;
        default:          str = NULL;               break;
    }
    return str;
}


> Can we throw some more specific SQLSTATE than the default "internal
> error" here?  Offhand I can't think of anything, but as a rule of
> thumb an ereport() ought to have an errcode().  If it really is an
> internal error then elog() is good enough.

Hmm.  I looked at the extant list, and found that the contrib/xml2 code
uses ERRCODE_EXTERNAL_ROUTINE_EXCEPTION whereas pgcrypto uses
ERRCODE_EXTERNAL_ROUTINE_INVOCATION_EXCEPTION.

In passing, I noticed the use of xml_ereport and xml_ereport_by_code in
the core XML code, and I'm left wondering whether we shouldn't add them
to the list of gettext triggers in nls.mk.  Also some messages there are
marked with ERRCODE_INTERNAL_ERROR.  (PLy_elog too?)

(Sorry, totally unrelated:) Wow, in plpython.c there is this:

static void *
PLy_malloc(size_t bytes)
{
    void       *ptr = malloc(bytes);

    if (ptr == NULL)
        ereport(FATAL,
                (errcode(ERRCODE_OUT_OF_MEMORY),
                 errmsg("out of memory")));
    return ptr;
}

I wonder why is it a good idea to have this report be FATAL.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> Can we throw some more specific SQLSTATE than the default "internal
>> error" here?

> Hmm.  I looked at the extant list, and found that the contrib/xml2 code
> uses ERRCODE_EXTERNAL_ROUTINE_EXCEPTION whereas pgcrypto uses
> ERRCODE_EXTERNAL_ROUTINE_INVOCATION_EXCEPTION.

Either of those would be fine with me; it's not totally clear what
distinction the SQL spec intends to draw.  The best I can divine is that
the latter is for bad arguments to the external routine, but we can't
really tell whether that's applicable.

> (Sorry, totally unrelated:) Wow, in plpython.c there is this:
> ...
> I wonder why is it a good idea to have this report be FATAL.

Yeah, I noticed that recently and was annoyed by it, but I'm not sure
how safe it is to change.  I suspect the author was worried about leaked
memory in event of a failure.

            regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> >     if (err != NULL)
> >         ereport(ERROR,
> >                 (errmsg("OSSP uuid failure: %s", err)));
> >     else
> >         ereport(ERROR,
> >                 (errmsg("OSSP uuid failure: error code %d", rc)));
>
> Maybe "OSSP uuid library failure"?  Otherwise seems OK.

Applied, thanks.

--
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.