Re: Bug in pg_restore memory handling

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(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073