Re: Bug in pg_restore memory handling - Mailing list pgsql-patches

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