Fix pg_restore's misdesigned code for detecting archive file format.

Despite the clear comments pointing out that the duplicative code
segments in ReadHead() and _discoverArchiveFormat() needed to be
in sync, they were not: the latter did not bother to apply any of
the sanity checks in the former.  We'd missed noticing this partly
because none of those checks would fail in scenarios we customarily
test, and partly because the oversight would be masked if both
segments execute, which they would in cases other than needing to
autodetect the format of a non-seekable stdin source.  However,
in a case meeting all these requirements --- for example, trying
to read a newer-than-supported archive format from non-seekable
stdin --- pg_restore missed applying the version check and would
likely dump core or otherwise misbehave.

The whole thing is silly anyway, because there seems little reason
to duplicate the logic beyond the one-line verification that the
file starts with "PGDMP".  There seems to have been an undocumented
assumption that multiple major formats (major enough to require
separate reader modules) would nonetheless share the first half-dozen
fields of the custom-format header.  This seems unlikely, so let's
fix it by just nuking the duplicate logic in _discoverArchiveFormat().

Also get rid of the pointless attempt to seek back to the start of
the file after successful autodetection.  That wastes cycles and
it means we have four behaviors to verify not two.

Per bug #16951 from Sergey Koposov.  This has been broken for
decades, so back-patch to all supported versions.

Discussion: https://postgr.es/m/16951-a4dd68cf0de23048@postgresql.org

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7c107ab5fcf2403125d0597da8a661b0afacc406..afcfb038bc19dc982380f2e078d36339315617c1 100644 (file)
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -2116,6 +2116,7 @@ _discoverArchiveFormat(ArchiveHandle *AH)
    if (AH->lookahead)
        free(AH->lookahead);
 
+   AH->readHeader = 0;
    AH->lookaheadSize = 512;
    AH->lookahead = pg_malloc0(512);
    AH->lookaheadLen = 0;
@@ -2189,62 +2190,9 @@ _discoverArchiveFormat(ArchiveHandle *AH)
 
    if (strncmp(sig, "PGDMP", 5) == 0)
    {
-       int         byteread;
-       char        vmaj,
-                   vmin,
-                   vrev;
-
-       /*
-        * Finish reading (most of) a custom-format header.
-        *
-        * NB: this code must agree with ReadHead().
-        */
-       if ((byteread = fgetc(fh)) == EOF)
-           READ_ERROR_EXIT(fh);
-
-       vmaj = byteread;
-
-       if ((byteread = fgetc(fh)) == EOF)
-           READ_ERROR_EXIT(fh);
-
-       vmin = byteread;
-
-       /* Save these too... */
-       AH->lookahead[AH->lookaheadLen++] = vmaj;
-       AH->lookahead[AH->lookaheadLen++] = vmin;
-
-       /* Check header version; varies from V1.0 */
-       if (vmaj > 1 || (vmaj == 1 && vmin > 0))    /* Version > 1.0 */
-       {
-           if ((byteread = fgetc(fh)) == EOF)
-               READ_ERROR_EXIT(fh);
-
-           vrev = byteread;
-           AH->lookahead[AH->lookaheadLen++] = vrev;
-       }
-       else
-           vrev = 0;
-
-       AH->version = MAKE_ARCHIVE_VERSION(vmaj, vmin, vrev);
-
-       if ((AH->intSize = fgetc(fh)) == EOF)
-           READ_ERROR_EXIT(fh);
-       AH->lookahead[AH->lookaheadLen++] = AH->intSize;
-
-       if (AH->version >= K_VERS_1_7)
-       {
-           if ((AH->offSize = fgetc(fh)) == EOF)
-               READ_ERROR_EXIT(fh);
-           AH->lookahead[AH->lookaheadLen++] = AH->offSize;
-       }
-       else
-           AH->offSize = AH->intSize;
-
-       if ((byteread = fgetc(fh)) == EOF)
-           READ_ERROR_EXIT(fh);
-
-       AH->format = byteread;
-       AH->lookahead[AH->lookaheadLen++] = AH->format;
+       /* It's custom format, stop here */
+       AH->format = archCustom;
+       AH->readHeader = 1;
    }
    else
    {
@@ -2281,23 +2229,16 @@ _discoverArchiveFormat(ArchiveHandle *AH)
        AH->format = archTar;
    }
 
-   /* If we can't seek, then mark the header as read */
-   if (fseeko(fh, 0, SEEK_SET) != 0)
-   {
-       /*
-        * NOTE: Formats that use the lookahead buffer can unset this in their
-        * Init routine.
-        */
-       AH->readHeader = 1;
-   }
-   else
-       AH->lookaheadLen = 0;   /* Don't bother since we've reset the file */
-
-   /* Close the file */
+   /* Close the file if we opened it */
    if (wantClose)
+   {
        if (fclose(fh) != 0)
            exit_horribly(modulename, "could not close input file: %s\n",
                          strerror(errno));
+       /* Forget lookahead, since we'll re-read header after re-opening */
+       AH->readHeader = 0;
+       AH->lookaheadLen = 0;
+   }
 
    return AH->format;
 }
@@ -3730,7 +3671,9 @@ WriteHead(ArchiveHandle *AH)
 void
 ReadHead(ArchiveHandle *AH)
 {
-   char        tmpMag[7];
+   char        vmaj,
+               vmin,
+               vrev;
    int         fmt;
    struct tm   crtm;
 
@@ -3742,48 +3685,46 @@ ReadHead(ArchiveHandle *AH)
     */
    if (!AH->readHeader)
    {
-       char        vmaj,
-                   vmin,
-                   vrev;
+       char        tmpMag[7];
 
        (*AH->ReadBufPtr) (AH, tmpMag, 5);
 
        if (strncmp(tmpMag, "PGDMP", 5) != 0)
            exit_horribly(modulename, "did not find magic string in file header\n");
+   }
 
-       vmaj = (*AH->ReadBytePtr) (AH);
-       vmin = (*AH->ReadBytePtr) (AH);
+   vmaj = (*AH->ReadBytePtr) (AH);
+   vmin = (*AH->ReadBytePtr) (AH);
 
-       if (vmaj > 1 || (vmaj == 1 && vmin > 0))    /* Version > 1.0 */
-           vrev = (*AH->ReadBytePtr) (AH);
-       else
-           vrev = 0;
+   if (vmaj > 1 || (vmaj == 1 && vmin > 0))    /* Version > 1.0 */
+       vrev = (*AH->ReadBytePtr) (AH);
+   else
+       vrev = 0;
 
-       AH->version = MAKE_ARCHIVE_VERSION(vmaj, vmin, vrev);
+   AH->version = MAKE_ARCHIVE_VERSION(vmaj, vmin, vrev);
 
-       if (AH->version < K_VERS_1_0 || AH->version > K_VERS_MAX)
-           exit_horribly(modulename, "unsupported version (%d.%d) in file header\n",
-                         vmaj, vmin);
+   if (AH->version < K_VERS_1_0 || AH->version > K_VERS_MAX)
+       exit_horribly(modulename, "unsupported version (%d.%d) in file header\n",
+                     vmaj, vmin);
 
-       AH->intSize = (*AH->ReadBytePtr) (AH);
-       if (AH->intSize > 32)
-           exit_horribly(modulename, "sanity check on integer size (%lu) failed\n",
-                         (unsigned long) AH->intSize);
+   AH->intSize = (*AH->ReadBytePtr) (AH);
+   if (AH->intSize > 32)
+       exit_horribly(modulename, "sanity check on integer size (%lu) failed\n",
+                     (unsigned long) AH->intSize);
 
-       if (AH->intSize > sizeof(int))
-           write_msg(modulename, "WARNING: archive was made on a machine with larger integers, some operations might fail\n");
+   if (AH->intSize > sizeof(int))
+       write_msg(modulename, "WARNING: archive was made on a machine with larger integers, some operations might fail\n");
 
-       if (AH->version >= K_VERS_1_7)
-           AH->offSize = (*AH->ReadBytePtr) (AH);
-       else
-           AH->offSize = AH->intSize;
+   if (AH->version >= K_VERS_1_7)
+       AH->offSize = (*AH->ReadBytePtr) (AH);
+   else
+       AH->offSize = AH->intSize;
 
-       fmt = (*AH->ReadBytePtr) (AH);
+   fmt = (*AH->ReadBytePtr) (AH);
 
-       if (AH->format != fmt)
-           exit_horribly(modulename, "expected format (%d) differs from format found in file (%d)\n",
-                         AH->format, fmt);
-   }
+   if (AH->format != fmt)
+       exit_horribly(modulename, "expected format (%d) differs from format found in file (%d)\n",
+                     AH->format, fmt);
 
    if (AH->version >= K_VERS_1_2)
    {

diff --git a/src/bin/pg_dump/pg_backup_archiver.h b/src/bin/pg_dump/pg_backup_archiver.h
index ab86a71ce4b06b0c207e9d48d3fb0dc62afa1161..5c07c41b809259cb35a930b4eda9af52f2ca5490 100644 (file)
--- a/src/bin/pg_dump/pg_backup_archiver.h
+++ b/src/bin/pg_dump/pg_backup_archiver.h
@@ -262,15 +262,21 @@ struct _archiveHandle
    time_t      createDate;     /* Date archive created */
 
    /*
-    * Fields used when discovering header. A format can always get the
-    * previous read bytes from here...
+    * Fields used when discovering archive format.  For tar format, we load
+    * the first block into the lookahead buffer, and verify that it looks
+    * like a tar header.  The tar module must then consume bytes from the
+    * lookahead buffer before reading any more from the file.  For custom
+    * format, we load only the "PGDMP" marker into the buffer, and then set
+    * readHeader after confirming it matches.  The buffer is vestigial in
+    * this case, as the subsequent code just checks readHeader and doesn't
+    * examine the buffer.
     */
-   int         readHeader;     /* Used if file header has been read already */
+   int         readHeader;     /* Set if we already read "PGDMP" marker */
    char       *lookahead;      /* Buffer used when reading header to discover
                                 * format */
-   size_t      lookaheadSize;  /* Size of allocated buffer */
-   size_t      lookaheadLen;   /* Length of data in lookahead */
-   pgoff_t     lookaheadPos;   /* Current read position in lookahead buffer */
+   size_t      lookaheadSize;  /* Allocated size of buffer */
+   size_t      lookaheadLen;   /* Length of valid data in lookahead */
+   size_t      lookaheadPos;   /* Current read position in lookahead buffer */
 
    ArchiveEntryPtrType ArchiveEntryPtr;    /* Called for each metadata object */
    StartDataPtrType StartDataPtr;  /* Called when table data is about to be

diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index 9226588ff28cea7d174d65751a12b1f1f90de413..3ba0db664bb20d3f0a4df8615ca71419a9140223 100644 (file)
--- a/src/bin/pg_dump/pg_backup_tar.c
+++ b/src/bin/pg_dump/pg_backup_tar.c
@@ -236,12 +236,6 @@ InitArchiveFmt_Tar(ArchiveHandle *AH)
 
        ctx->hasSeek = checkSeek(ctx->tarFH);
 
-       /*
-        * Forcibly unmark the header as read since we use the lookahead
-        * buffer
-        */
-       AH->readHeader = 0;
-
        ctx->FH = (void *) tarOpen(AH, "toc.dat", 'r');
        ReadHead(AH);
        ReadToc(AH);