Potential NULL dereference found in typecmds.c

Hi folks,

Sentry found this error last night, and it looks serious enough to
report. The error was introduced in commit 426cafc. Here's the code
in question, starting at line 2096:

if (!found)
{
con = NULL; /* keep compiler quiet */
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
errmsg("constraint \"%s\" of domain \"%s\" does not exist",
constrName, NameStr(con->conname))));
}

It sets 'con' to NULL and then in the next statement, dereferences it.
I'm not sure if it's possible to reach this path, but if it is
reachable it will cause a crash.

Best Regards,
Mike

--
Mike Mueller
Phone: (401) 405-1525
Email: mmueller(at)vigilantsw(dot)com

http://www.vigilantsw.com/

On Sat, Jul 2, 2011 at 20:10, Michael Mueller <mmueller(at)vigilantsw(dot)com> wrote:
> Hi folks,
>
> Sentry found this error last night, and it looks serious enough to
> report.  The error was introduced in commit 426cafc.  Here's the code
> in question, starting at line 2096:
>
>    if (!found)
>    {
>        con = NULL;     /* keep compiler quiet */
>        ereport(ERROR,
>                (errcode(ERRCODE_UNDEFINED_OBJECT),
>                 errmsg("constraint \"%s\" of domain \"%s\" does not exist",
>                        constrName, NameStr(con->conname))));
>    }
>
> It sets 'con' to NULL and then in the next statement, dereferences it.
> I'm not sure if it's possible to reach this path, but if it is
> reachable it will cause a crash.

This code is no longer present in git head, *removed* by commit
426cafc. Not added by it. at least that's how I read the history...

However, it still looks to me like we could get to that code with
con=NULL - if the while loop is never executed. Perhaps this is a
can-never-happen situation? Alvaro?

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

On 4 July 2011 13:53, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> This code is no longer present in git head, *removed* by commit
> 426cafc. Not added by it. at least that's how I read the history...
>
> However, it still looks to me like we could get to that code with
> con=NULL - if the while loop is never executed. Perhaps this is a
> can-never-happen situation? Alvaro?

Seems slightly academic IMHO. No code path should dereference an
invariably NULL or wild pointer.

--
Peter Geoghegan       http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training and Services

On 04.07.2011 16:07, Peter Geoghegan wrote:
> On 4 July 2011 13:53, Magnus Hagander<magnus(at)hagander(dot)net> wrote:
>> This code is no longer present in git head, *removed* by commit
>> 426cafc. Not added by it. at least that's how I read the history...
>>
>> However, it still looks to me like we could get to that code with
>> con=NULL - if the while loop is never executed. Perhaps this is a
>> can-never-happen situation? Alvaro?
>
> Seems slightly academic IMHO. No code path should dereference an
> invariably NULL or wild pointer.

That error message is bogus anyway:

> if (!found)
> ereport(ERROR,
> (errcode(ERRCODE_UNDEFINED_OBJECT),
> errmsg("constraint \"%s\" of domain \"%s\" does not exist",
> constrName, NameStr(con->conname))));

It passes con->conname as the name of the domain, which is just wrong.
It should be TypeNameToString(typename) instead. The second error
message that follows has the same bug.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Excerpts from Heikki Linnakangas's message of lun jul 04 09:14:11 -0400 2011:
> On 04.07.2011 16:07, Peter Geoghegan wrote:

> That error message is bogus anyway:
>
> > if (!found)
> > ereport(ERROR,
> > (errcode(ERRCODE_UNDEFINED_OBJECT),
> > errmsg("constraint \"%s\" of domain \"%s\" does not exist",
> > constrName, NameStr(con->conname))));
>
> It passes con->conname as the name of the domain, which is just wrong.
> It should be TypeNameToString(typename) instead. The second error
> message that follows has the same bug.

Um, evidently this code has a problem. I'll fix.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Excerpts from Alvaro Herrera's message of lun jul 04 11:12:32 -0400 2011:
> Excerpts from Heikki Linnakangas's message of lun jul 04 09:14:11 -0400 2011:
> > On 04.07.2011 16:07, Peter Geoghegan wrote:
>
> > That error message is bogus anyway:
> >
> > > if (!found)
> > > ereport(ERROR,
> > > (errcode(ERRCODE_UNDEFINED_OBJECT),
> > > errmsg("constraint \"%s\" of domain \"%s\" does not exist",
> > > constrName, NameStr(con->conname))));
> >
> > It passes con->conname as the name of the domain, which is just wrong.
> > It should be TypeNameToString(typename) instead. The second error
> > message that follows has the same bug.
>
> Um, evidently this code has a problem. I'll fix.

I forgot to close this: I applied Heikki's suggested fix yesterday.
Thanks for the report.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support