Thread: Error in recent pg_dump change (coverity)

Error in recent pg_dump change (coverity)

From
Martijn van Oosterhout
Date:
Coverity picked up an error in dumpStdStrings() since last night. At
line 1448 there's PQclear(res) yet it's used several times further down
(lines 1452, 1454 and 1456).

I'd actually suggest zeroing out res->tuples in PQclear so this sort of
problem becomes much more obvious.

Coverity bug 304 for people watching at home.
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Re: Error in recent pg_dump change (coverity)

From
Alvaro Herrera
Date:
Martijn van Oosterhout wrote:
> Coverity picked up an error in dumpStdStrings() since last night. At
> line 1448 there's PQclear(res) yet it's used several times further down
> (lines 1452, 1454 and 1456).
>
> I'd actually suggest zeroing out res->tuples in PQclear so this sort of
> problem becomes much more obvious.

Is it worthwhile to zero out the res->block array as well?

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

Attachment

Re: Error in recent pg_dump change (coverity)

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Martijn van Oosterhout wrote:
>> I'd actually suggest zeroing out res->tuples in PQclear so this sort of
>> problem becomes much more obvious.

> Is it worthwhile to zero out the res->block array as well?

Your patch isn't doing that, merely zeroing a local variable
that will be assigned to in a moment anyway.  That loop already
ensures that res->curBlock is null when it exits.  So lose this:

+         block = NULL;

This part seems OK:
     /* Free the top-level tuple pointer array */     if (res->tuples)
+     {         free(res->tuples);
+         res->tuples = NULL;
+     }

Another possibility is to just MemSet the whole PGresult struct
to zeroes before free'ing it.  Compared to the cost of obtaining
a query result from the backend, this probably doesn't cost enough
to be worth worrying about, and it would catch a few more problems
of the same ilk.
        regards, tom lane


Re: Error in recent pg_dump change (coverity)

From
Martijn van Oosterhout
Date:
On Sun, May 28, 2006 at 11:42:48AM -0400, Alvaro Herrera wrote:
> Martijn van Oosterhout wrote:
> > I'd actually suggest zeroing out res->tuples in PQclear so this sort of
> > problem becomes much more obvious.
>
> Is it worthwhile to zero out the res->block array as well?

Patch is good, but setting block to NULL is a waste of time, it's not
global. And I think res->curBlock is automatically set NULL as result
of the loop...

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Re: Error in recent pg_dump change (coverity)

From
Martijn van Oosterhout
Date:
On Sun, May 28, 2006 at 12:00:33PM -0400, Tom Lane wrote:
> Another possibility is to just MemSet the whole PGresult struct
> to zeroes before free'ing it.  Compared to the cost of obtaining
> a query result from the backend, this probably doesn't cost enough
> to be worth worrying about, and it would catch a few more problems
> of the same ilk.

Probably better actually, since by setting ntups to zero also,
PQgetvalue will return a warning (row number out of range) rather than
segfaulting...

Have a nice day,
--
Martijn van Oosterhout   <kleptog@svana.org>   http://svana.org/kleptog/
> From each according to his ability. To each according to his ability to litigate.

Re: Error in recent pg_dump change (coverity)

From
Tom Lane
Date:
Martijn van Oosterhout <kleptog@svana.org> writes:
> On Sun, May 28, 2006 at 12:00:33PM -0400, Tom Lane wrote:
>> Another possibility is to just MemSet the whole PGresult struct
>> to zeroes before free'ing it.

> Probably better actually, since by setting ntups to zero also,
> PQgetvalue will return a warning (row number out of range) rather than
> segfaulting...

Hm.  But I think we'd *like* it to segfault; the idea is to make the
user's programming error as obvious as possible.  Is it worth the
trouble to just zero out the pointer members of the PGresult?
        regards, tom lane


Re: Error in recent pg_dump change (coverity)

From
Alvaro Herrera
Date:
Tom Lane wrote:
> Martijn van Oosterhout <kleptog@svana.org> writes:
> > On Sun, May 28, 2006 at 12:00:33PM -0400, Tom Lane wrote:
> >> Another possibility is to just MemSet the whole PGresult struct
> >> to zeroes before free'ing it.
> 
> > Probably better actually, since by setting ntups to zero also,
> > PQgetvalue will return a warning (row number out of range) rather than
> > segfaulting...
> 
> Hm.  But I think we'd *like* it to segfault; the idea is to make the
> user's programming error as obvious as possible.  Is it worth the
> trouble to just zero out the pointer members of the PGresult?

There are only five of them; four need to be zeroed out.


void
PQclear(PGresult *res)
{   PGresult_data *block;
   if (!res)       return;      /* Free all the subsidiary blocks */   while ((block = res->curBlock) != NULL)   {
res->curBlock = block->next;       free(block);   }
 
   /* Free the top-level tuple pointer array */   if (res->tuples)       free(res->tuples);
   /* zero out the pointer fields to catch programming errors */   res->attDesc = NULL;   res->tuples = NULL;
res->noticeHooks= NULL;   res->errFields = NULL;   /* res->curBlock was zeroed out earlier */      /* Free the PGresult
structureitself */   free(res);
 
}

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


Re: Error in recent pg_dump change (coverity)

From
Tom Lane
Date:
Alvaro Herrera <alvherre@commandprompt.com> writes:
> Tom Lane wrote:
>> Hm.  But I think we'd *like* it to segfault; the idea is to make the
>> user's programming error as obvious as possible.  Is it worth the
>> trouble to just zero out the pointer members of the PGresult?

> There are only five of them; four need to be zeroed out.

Works for me.  Please commit, as I'm about to do some further work in
those files and would rather not have to merge ...
        regards, tom lane


Re: Error in recent pg_dump change (coverity)

From
Alvaro Herrera
Date:
Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Tom Lane wrote:
> >> Hm.  But I think we'd *like* it to segfault; the idea is to make the
> >> user's programming error as obvious as possible.  Is it worth the
> >> trouble to just zero out the pointer members of the PGresult?
> 
> > There are only five of them; four need to be zeroed out.
> 
> Works for me.  Please commit, as I'm about to do some further work in
> those files and would rather not have to merge ...

Done.  They were actually four, not five.  The one I mistakingly though
was one was the notice processor hooks.

The case Martijn was saying would be warned about by the memset
approach, setting ntuples to 0, would actually be handled as a segfault,
because functions like check_field_number actually follow
res.noticeHooks pointer!  ISTM we would just segfault at that point.

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


Re: Error in recent pg_dump change (coverity)

From
Bruce Momjian
Date:
Alvaro Herrera wrote:
> Tom Lane wrote:
> > Alvaro Herrera <alvherre@commandprompt.com> writes:
> > > Tom Lane wrote:
> > >> Hm.  But I think we'd *like* it to segfault; the idea is to make the
> > >> user's programming error as obvious as possible.  Is it worth the
> > >> trouble to just zero out the pointer members of the PGresult?
> > 
> > > There are only five of them; four need to be zeroed out.
> > 
> > Works for me.  Please commit, as I'm about to do some further work in
> > those files and would rather not have to merge ...
> 
> Done.  They were actually four, not five.  The one I mistakingly though
> was one was the notice processor hooks.
> 
> The case Martijn was saying would be warned about by the memset
> approach, setting ntuples to 0, would actually be handled as a segfault,
> because functions like check_field_number actually follow
> res.noticeHooks pointer!  ISTM we would just segfault at that point.

Agreed.  Anything to catch more errors is good.

--  Bruce Momjian   http://candle.pha.pa.us EnterpriseDB    http://www.enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +


Re: Error in recent pg_dump change (coverity)

From
Alvaro Herrera
Date:
Alvaro Herrera wrote:

> Done.  They were actually four, not five.  The one I mistakingly though
> was one was the notice processor hooks.
> 
> The case Martijn was saying would be warned about by the memset
> approach, setting ntuples to 0, would actually be handled as a segfault,
> because functions like check_field_number actually follow
> res.noticeHooks pointer!  ISTM we would just segfault at that point.

I must be blind.  The hooks->noticeRec == NULL case is handled first
thing in pgInternalNotice (returns doing nothing).  So we wouldn't
segfault and we wouldn't emit any warning either!

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