Thread: Error in recent pg_dump change (coverity)
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.
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
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
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.
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.
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
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.
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
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.
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. +
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.