Thread: Bug in pg_restore memory handling
I found a bug in the pg_restore code. It shows up only using the tar
format, and only on Windows XP (not Win2000 or BSD/OS). However, the
bug exists on all platforms that don't return zero'ed memory from
malloc, which is basically everyone. We have gotten away with this
because malloc memory is usually zeroed by accident in pg_dump (I think
because it is an early malloc call, not recycled from a free.)
The bug seems to only affect the output displayed by pg_restore --- the
data seems to restore fine. To test, try:
pg_dump -Ft test >/tmp/test.db
pg_dump /tmp/test.db
For a simple case, you should see displayed by pg_restore:
COPY supplier (sno, sname, city) FROM stdin;
1 Smith London
2 Jones Paris
3 Adams Vienna
4 Blake Roma
\.
But on XP with the bug I see:
COPY supplier (sno, sname, city) FROM stdin;
\.
copy supplier (sno, sname, city) from '$$PATH$$/6.dat' ;
The code in pg_backup_tar.c::InitArchiveFmt_Tar does:
ctx = (lclContext *) malloc(sizeof(lclContext));
AH->formatData = (void *) ctx;
ctx->filePos = 0;
What it doesn't do is to set ctx->isSpecialScript to zero:
ctx->isSpecialScript = 0;
pg_backup_tar::_PrintTocData() checks ctx->isSpecialScript for non-zero,
and then uses the wrong code path on XP. The code is supposed to use
the ctx->isSpecialScript code path only after the archive is closed.
This is set to true in _CloseArchive(). ctx->isSpecialScript is used
only for tar, so that's why only tar has the bug.
A simple patch would be to just add the ctx->isSpecialScript = 0, but a
more reliable patch would be to do that, plus change a few malloc calls
to calloc for complex structures where the initialization isn't clear in
the code.
I would like to apply the attached patch to 7.3.X and 7.4.
Comments?
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073
Index: src/bin/pg_dump/pg_backup_custom.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_backup_custom.c,v
retrieving revision 1.25
diff -c -c -r1.25 pg_backup_custom.c
*** src/bin/pg_dump/pg_backup_custom.c 4 Aug 2003 00:43:27 -0000 1.25
--- src/bin/pg_dump/pg_backup_custom.c 6 Oct 2003 18:00:40 -0000
***************
*** 136,142 ****
/*
* Set up some special context used in compressing data.
*/
! ctx = (lclContext *) malloc(sizeof(lclContext));
if (ctx == NULL)
die_horribly(AH, modulename, "out of memory\n");
AH->formatData = (void *) ctx;
--- 136,142 ----
/*
* Set up some special context used in compressing data.
*/
! ctx = (lclContext *) calloc(1, sizeof(lclContext));
if (ctx == NULL)
die_horribly(AH, modulename, "out of memory\n");
AH->formatData = (void *) ctx;
***************
*** 253,259 ****
if (ctx == NULL)
{
! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry));
te->formatData = (void *) ctx;
}
--- 253,259 ----
if (ctx == NULL)
{
! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry));
te->formatData = (void *) ctx;
}
Index: src/bin/pg_dump/pg_backup_files.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_backup_files.c,v
retrieving revision 1.21
diff -c -c -r1.21 pg_backup_files.c
*** src/bin/pg_dump/pg_backup_files.c 25 Oct 2002 01:33:17 -0000 1.21
--- src/bin/pg_dump/pg_backup_files.c 6 Oct 2003 18:00:40 -0000
***************
*** 101,107 ****
/*
* Set up some special context used in compressing data.
*/
! ctx = (lclContext *) malloc(sizeof(lclContext));
AH->formatData = (void *) ctx;
ctx->filePos = 0;
--- 101,107 ----
/*
* Set up some special context used in compressing data.
*/
! ctx = (lclContext *) calloc(1, sizeof(lclContext));
AH->formatData = (void *) ctx;
ctx->filePos = 0;
***************
*** 167,173 ****
lclTocEntry *ctx;
char fn[K_STD_BUF_SIZE];
! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry));
if (te->dataDumper)
{
#ifdef HAVE_LIBZ
--- 167,173 ----
lclTocEntry *ctx;
char fn[K_STD_BUF_SIZE];
! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry));
if (te->dataDumper)
{
#ifdef HAVE_LIBZ
***************
*** 206,212 ****
if (ctx == NULL)
{
! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry));
te->formatData = (void *) ctx;
}
--- 206,212 ----
if (ctx == NULL)
{
! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry));
te->formatData = (void *) ctx;
}
Index: src/bin/pg_dump/pg_backup_tar.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_backup_tar.c,v
retrieving revision 1.37
diff -c -c -r1.37 pg_backup_tar.c
*** src/bin/pg_dump/pg_backup_tar.c 4 Aug 2003 00:43:27 -0000 1.37
--- src/bin/pg_dump/pg_backup_tar.c 6 Oct 2003 18:00:41 -0000
***************
*** 158,167 ****
/*
* Set up some special context used in compressing data.
*/
! ctx = (lclContext *) malloc(sizeof(lclContext));
AH->formatData = (void *) ctx;
ctx->filePos = 0;
!
/* Initialize LO buffering */
AH->lo_buf_size = LOBBUFSIZE;
AH->lo_buf = (void *) malloc(LOBBUFSIZE);
--- 158,168 ----
/*
* Set up some special context used in compressing data.
*/
! ctx = (lclContext *) calloc(1, sizeof(lclContext));
AH->formatData = (void *) ctx;
ctx->filePos = 0;
! ctx->isSpecialScript = 0;
!
/* Initialize LO buffering */
AH->lo_buf_size = LOBBUFSIZE;
AH->lo_buf = (void *) malloc(LOBBUFSIZE);
***************
*** 253,259 ****
lclTocEntry *ctx;
char fn[K_STD_BUF_SIZE];
! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry));
if (te->dataDumper != NULL)
{
#ifdef HAVE_LIBZ
--- 254,260 ----
lclTocEntry *ctx;
char fn[K_STD_BUF_SIZE];
! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry));
if (te->dataDumper != NULL)
{
#ifdef HAVE_LIBZ
***************
*** 292,298 ****
if (ctx == NULL)
{
! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry));
te->formatData = (void *) ctx;
}
--- 293,299 ----
if (ctx == NULL)
{
! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry));
te->formatData = (void *) ctx;
}
Index: src/bin/pg_dump/pg_dump.c
===================================================================
RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_dump.c,v
retrieving revision 1.352
diff -c -c -r1.352 pg_dump.c
*** src/bin/pg_dump/pg_dump.c 27 Sep 2003 22:10:01 -0000 1.352
--- src/bin/pg_dump/pg_dump.c 6 Oct 2003 18:00:47 -0000
***************
*** 1078,1084 ****
write_msg(NULL, "preparing to dump the contents of table %s\n",
classname);
! dumpCtx = (DumpContext *) malloc(sizeof(DumpContext));
dumpCtx->tblinfo = (TableInfo *) tblinfo;
dumpCtx->tblidx = i;
dumpCtx->oids = oids;
--- 1078,1084 ----
write_msg(NULL, "preparing to dump the contents of table %s\n",
classname);
! dumpCtx = (DumpContext *) calloc(1, sizeof(DumpContext));
dumpCtx->tblinfo = (TableInfo *) tblinfo;
dumpCtx->tblidx = i;
dumpCtx->oids = oids;
***************
*** 1938,1946 ****
*numFuncs = ntups;
! finfo = (FuncInfo *) malloc(ntups * sizeof(FuncInfo));
!
! memset((char *) finfo, 0, ntups * sizeof(FuncInfo));
i_oid = PQfnumber(res, "oid");
i_proname = PQfnumber(res, "proname");
--- 1938,1944 ----
*numFuncs = ntups;
! finfo = (FuncInfo *) calloc(ntups, sizeof(FuncInfo));
i_oid = PQfnumber(res, "oid");
i_proname = PQfnumber(res, "proname");
***************
*** 2144,2151 ****
* dumping only one, because we don't yet know which tables might be
* inheritance ancestors of the target table.
*/
! tblinfo = (TableInfo *) malloc(ntups * sizeof(TableInfo));
! memset(tblinfo, 0, ntups * sizeof(TableInfo));
i_reloid = PQfnumber(res, "oid");
i_relname = PQfnumber(res, "relname");
--- 2142,2148 ----
* dumping only one, because we don't yet know which tables might be
* inheritance ancestors of the target table.
*/
! tblinfo = (TableInfo *) calloc(ntups, sizeof(TableInfo));
i_reloid = PQfnumber(res, "oid");
i_relname = PQfnumber(res, "relname");
Patch applied. --------------------------------------------------------------------------- Bruce Momjian wrote: > I found a bug in the pg_restore code. It shows up only using the tar > format, and only on Windows XP (not Win2000 or BSD/OS). However, the > bug exists on all platforms that don't return zero'ed memory from > malloc, which is basically everyone. We have gotten away with this > because malloc memory is usually zeroed by accident in pg_dump (I think > because it is an early malloc call, not recycled from a free.) > > The bug seems to only affect the output displayed by pg_restore --- the > data seems to restore fine. To test, try: > > pg_dump -Ft test >/tmp/test.db > pg_dump /tmp/test.db > > For a simple case, you should see displayed by pg_restore: > > COPY supplier (sno, sname, city) FROM stdin; > 1 Smith London > 2 Jones Paris > 3 Adams Vienna > 4 Blake Roma > \. > > But on XP with the bug I see: > > COPY supplier (sno, sname, city) FROM stdin; > \. > copy supplier (sno, sname, city) from '$$PATH$$/6.dat' ; > > The code in pg_backup_tar.c::InitArchiveFmt_Tar does: > > ctx = (lclContext *) malloc(sizeof(lclContext)); > AH->formatData = (void *) ctx; > ctx->filePos = 0; > > What it doesn't do is to set ctx->isSpecialScript to zero: > > ctx->isSpecialScript = 0; > > pg_backup_tar::_PrintTocData() checks ctx->isSpecialScript for non-zero, > and then uses the wrong code path on XP. The code is supposed to use > the ctx->isSpecialScript code path only after the archive is closed. > This is set to true in _CloseArchive(). ctx->isSpecialScript is used > only for tar, so that's why only tar has the bug. > > A simple patch would be to just add the ctx->isSpecialScript = 0, but a > more reliable patch would be to do that, plus change a few malloc calls > to calloc for complex structures where the initialization isn't clear in > the code. > > I would like to apply the attached patch to 7.3.X and 7.4. > > Comments? > > -- > Bruce Momjian | http://candle.pha.pa.us > pgman@candle.pha.pa.us | (610) 359-1001 > + If your life is a hard drive, | 13 Roberts Road > + Christ can be your backup. | Newtown Square, Pennsylvania 19073 > Index: src/bin/pg_dump/pg_backup_custom.c > =================================================================== > RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_backup_custom.c,v > retrieving revision 1.25 > diff -c -c -r1.25 pg_backup_custom.c > *** src/bin/pg_dump/pg_backup_custom.c 4 Aug 2003 00:43:27 -0000 1.25 > --- src/bin/pg_dump/pg_backup_custom.c 6 Oct 2003 18:00:40 -0000 > *************** > *** 136,142 **** > /* > * Set up some special context used in compressing data. > */ > ! ctx = (lclContext *) malloc(sizeof(lclContext)); > if (ctx == NULL) > die_horribly(AH, modulename, "out of memory\n"); > AH->formatData = (void *) ctx; > --- 136,142 ---- > /* > * Set up some special context used in compressing data. > */ > ! ctx = (lclContext *) calloc(1, sizeof(lclContext)); > if (ctx == NULL) > die_horribly(AH, modulename, "out of memory\n"); > AH->formatData = (void *) ctx; > *************** > *** 253,259 **** > > if (ctx == NULL) > { > ! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry)); > te->formatData = (void *) ctx; > } > > --- 253,259 ---- > > if (ctx == NULL) > { > ! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry)); > te->formatData = (void *) ctx; > } > > Index: src/bin/pg_dump/pg_backup_files.c > =================================================================== > RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_backup_files.c,v > retrieving revision 1.21 > diff -c -c -r1.21 pg_backup_files.c > *** src/bin/pg_dump/pg_backup_files.c 25 Oct 2002 01:33:17 -0000 1.21 > --- src/bin/pg_dump/pg_backup_files.c 6 Oct 2003 18:00:40 -0000 > *************** > *** 101,107 **** > /* > * Set up some special context used in compressing data. > */ > ! ctx = (lclContext *) malloc(sizeof(lclContext)); > AH->formatData = (void *) ctx; > ctx->filePos = 0; > > --- 101,107 ---- > /* > * Set up some special context used in compressing data. > */ > ! ctx = (lclContext *) calloc(1, sizeof(lclContext)); > AH->formatData = (void *) ctx; > ctx->filePos = 0; > > *************** > *** 167,173 **** > lclTocEntry *ctx; > char fn[K_STD_BUF_SIZE]; > > ! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry)); > if (te->dataDumper) > { > #ifdef HAVE_LIBZ > --- 167,173 ---- > lclTocEntry *ctx; > char fn[K_STD_BUF_SIZE]; > > ! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry)); > if (te->dataDumper) > { > #ifdef HAVE_LIBZ > *************** > *** 206,212 **** > > if (ctx == NULL) > { > ! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry)); > te->formatData = (void *) ctx; > } > > --- 206,212 ---- > > if (ctx == NULL) > { > ! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry)); > te->formatData = (void *) ctx; > } > > Index: src/bin/pg_dump/pg_backup_tar.c > =================================================================== > RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_backup_tar.c,v > retrieving revision 1.37 > diff -c -c -r1.37 pg_backup_tar.c > *** src/bin/pg_dump/pg_backup_tar.c 4 Aug 2003 00:43:27 -0000 1.37 > --- src/bin/pg_dump/pg_backup_tar.c 6 Oct 2003 18:00:41 -0000 > *************** > *** 158,167 **** > /* > * Set up some special context used in compressing data. > */ > ! ctx = (lclContext *) malloc(sizeof(lclContext)); > AH->formatData = (void *) ctx; > ctx->filePos = 0; > ! > /* Initialize LO buffering */ > AH->lo_buf_size = LOBBUFSIZE; > AH->lo_buf = (void *) malloc(LOBBUFSIZE); > --- 158,168 ---- > /* > * Set up some special context used in compressing data. > */ > ! ctx = (lclContext *) calloc(1, sizeof(lclContext)); > AH->formatData = (void *) ctx; > ctx->filePos = 0; > ! ctx->isSpecialScript = 0; > ! > /* Initialize LO buffering */ > AH->lo_buf_size = LOBBUFSIZE; > AH->lo_buf = (void *) malloc(LOBBUFSIZE); > *************** > *** 253,259 **** > lclTocEntry *ctx; > char fn[K_STD_BUF_SIZE]; > > ! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry)); > if (te->dataDumper != NULL) > { > #ifdef HAVE_LIBZ > --- 254,260 ---- > lclTocEntry *ctx; > char fn[K_STD_BUF_SIZE]; > > ! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry)); > if (te->dataDumper != NULL) > { > #ifdef HAVE_LIBZ > *************** > *** 292,298 **** > > if (ctx == NULL) > { > ! ctx = (lclTocEntry *) malloc(sizeof(lclTocEntry)); > te->formatData = (void *) ctx; > } > > --- 293,299 ---- > > if (ctx == NULL) > { > ! ctx = (lclTocEntry *) calloc(1, sizeof(lclTocEntry)); > te->formatData = (void *) ctx; > } > > Index: src/bin/pg_dump/pg_dump.c > =================================================================== > RCS file: /cvsroot/pgsql-server/src/bin/pg_dump/pg_dump.c,v > retrieving revision 1.352 > diff -c -c -r1.352 pg_dump.c > *** src/bin/pg_dump/pg_dump.c 27 Sep 2003 22:10:01 -0000 1.352 > --- src/bin/pg_dump/pg_dump.c 6 Oct 2003 18:00:47 -0000 > *************** > *** 1078,1084 **** > write_msg(NULL, "preparing to dump the contents of table %s\n", > classname); > > ! dumpCtx = (DumpContext *) malloc(sizeof(DumpContext)); > dumpCtx->tblinfo = (TableInfo *) tblinfo; > dumpCtx->tblidx = i; > dumpCtx->oids = oids; > --- 1078,1084 ---- > write_msg(NULL, "preparing to dump the contents of table %s\n", > classname); > > ! dumpCtx = (DumpContext *) calloc(1, sizeof(DumpContext)); > dumpCtx->tblinfo = (TableInfo *) tblinfo; > dumpCtx->tblidx = i; > dumpCtx->oids = oids; > *************** > *** 1938,1946 **** > > *numFuncs = ntups; > > ! finfo = (FuncInfo *) malloc(ntups * sizeof(FuncInfo)); > ! > ! memset((char *) finfo, 0, ntups * sizeof(FuncInfo)); > > i_oid = PQfnumber(res, "oid"); > i_proname = PQfnumber(res, "proname"); > --- 1938,1944 ---- > > *numFuncs = ntups; > > ! finfo = (FuncInfo *) calloc(ntups, sizeof(FuncInfo)); > > i_oid = PQfnumber(res, "oid"); > i_proname = PQfnumber(res, "proname"); > *************** > *** 2144,2151 **** > * dumping only one, because we don't yet know which tables might be > * inheritance ancestors of the target table. > */ > ! tblinfo = (TableInfo *) malloc(ntups * sizeof(TableInfo)); > ! memset(tblinfo, 0, ntups * sizeof(TableInfo)); > > i_reloid = PQfnumber(res, "oid"); > i_relname = PQfnumber(res, "relname"); > --- 2142,2148 ---- > * dumping only one, because we don't yet know which tables might be > * inheritance ancestors of the target table. > */ > ! tblinfo = (TableInfo *) calloc(ntups, sizeof(TableInfo)); > > i_reloid = PQfnumber(res, "oid"); > i_relname = PQfnumber(res, "relname"); > > ---------------------------(end of broadcast)--------------------------- > TIP 6: Have you searched our list archives? > > http://archives.postgresql.org -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania 19073