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/
Show quoted text
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
Attachments:
coverity-304.patchtext/plain; charset=us-asciiDownload
Index: src/bin/pg_dump/pg_dump.c
===================================================================
RCS file: /home/alvherre/cvs/pgsql/src/bin/pg_dump/pg_dump.c,v
retrieving revision 1.434
diff -c -r1.434 pg_dump.c
*** src/bin/pg_dump/pg_dump.c 26 May 2006 23:48:54 -0000 1.434
--- src/bin/pg_dump/pg_dump.c 28 May 2006 15:39:14 -0000
***************
*** 1445,1452 ****
check_sql_result(res, g_conn, qry->data, PGRES_TUPLES_OK);
- PQclear(res);
-
resetPQExpBuffer(qry);
std_strings = (strcmp(PQgetvalue(res, 0, 0), "on") == 0);
--- 1445,1450 ----
***************
*** 1454,1460 ****
appendStringLiteral(qry, PQgetvalue(res, 0, 0), true, !std_strings);
appendPQExpBuffer(qry, ";\n");
puts(PQgetvalue(res, 0, 0));
!
}
ArchiveEntry(AH, nilCatalogId, createDumpId(),
--- 1452,1459 ----
appendStringLiteral(qry, PQgetvalue(res, 0, 0), true, !std_strings);
appendPQExpBuffer(qry, ";\n");
puts(PQgetvalue(res, 0, 0));
!
! PQclear(res);
}
ArchiveEntry(AH, nilCatalogId, createDumpId(),
Index: src/interfaces/libpq/fe-exec.c
===================================================================
RCS file: /home/alvherre/cvs/pgsql/src/interfaces/libpq/fe-exec.c,v
retrieving revision 1.184
diff -c -r1.184 fe-exec.c
*** src/interfaces/libpq/fe-exec.c 23 May 2006 22:13:19 -0000 1.184
--- src/interfaces/libpq/fe-exec.c 28 May 2006 15:39:20 -0000
***************
*** 358,368 ****
--- 358,372 ----
{
res->curBlock = block->next;
free(block);
+ block = NULL;
}
/* Free the top-level tuple pointer array */
if (res->tuples)
+ {
free(res->tuples);
+ res->tuples = NULL;
+ }
/* Free the PGresult structure itself */
free(res);
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/
Show quoted text
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/
Show quoted text
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 structure itself */
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.