Bug in pg_restore memory handling - Mailing list pgsql-patches

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");