Re: pg_verifybackup: TAR format backup verification

On Sun, Sep 29, 2024 at 1:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> *** CID 1620458:  Resource leaks  (RESOURCE_LEAK)
> /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c: 1025 in verify_tar_file()
> 1019                                                            relpath);
> 1020
> 1021            /* Close the file. */
> 1022            if (close(fd) != 0)
> 1023                    report_backup_error(context, "could not close file \"%s\": %m",
> 1024                                                            relpath);
> >>>     CID 1620458:  Resource leaks  (RESOURCE_LEAK)
> >>>     Variable "buffer" going out of scope leaks the storage it points to.
> 1025     }
> 1026
> 1027     /*
> 1028      * Scan the hash table for entries where the 'matched' flag is not set; report
> 1029      * that such files are present in the manifest but not on disk.
> 1030      */

This looks like a real leak. It can only happen once per tarfile when
verifying a tar backup so it can never add up to much, but it makes
sense to fix it.

> *** CID 1620457:  Memory - illegal accesses  (OVERRUN)
> /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/astreamer_verify.c: 349 in member_copy_control_data()
> 343              */
> 344             if (mystreamer->control_file_bytes <= sizeof(ControlFileData))
> 345             {
> 346                     int                     remaining;
> 347
> 348                     remaining = sizeof(ControlFileData) - mystreamer->control_file_bytes;
> >>>     CID 1620457:  Memory - illegal accesses  (OVERRUN)
> >>>     Overrunning array of 296 bytes at byte offset 296 by dereferencing pointer "(char *)&mystreamer->control_file + mystreamer->control_file_bytes".
> 349                     memcpy(((char *) &mystreamer->control_file)
> 350                                + mystreamer->control_file_bytes,
> 351                                data, Min(len, remaining));
> 352             }
> 353
> 354             /* Remember how many bytes we saw, even if we didn't buffer them. */

I think this might be complaining about a potential zero-length copy.
Seems like perhaps the <= sizeof(ControlFileData) test should actually
be < sizeof(ControlFileData).

> *** CID 1620456:  Null pointer dereferences  (FORWARD_NULL)
> /srv/coverity/git/pgsql-git/postgresql/src/bin/pg_verifybackup/pg_verifybackup.c: 939 in precheck_tar_backup_file()
> 933                                                                     "file \"%s\" is not expected in a tar format backup",
> 934                                                                     relpath);
> 935                     tblspc_oid = (Oid) num;
> 936             }
> 937
> 938             /* Now, check the compression type of the tar */
> >>>     CID 1620456:  Null pointer dereferences  (FORWARD_NULL)
> >>>     Passing null pointer "suffix" to "strcmp", which dereferences it.
> 939             if (strcmp(suffix, ".tar") == 0)
> 940                     compress_algorithm = PG_COMPRESSION_NONE;
> 941             else if (strcmp(suffix, ".tgz") == 0)
> 942                     compress_algorithm = PG_COMPRESSION_GZIP;
> 943             else if (strcmp(suffix, ".tar.gz") == 0)
> 944                     compress_algorithm = PG_COMPRESSION_GZIP;

This one is happening, I believe, because report_backup_error()
doesn't perform a non-local exit, but we have a bit of code here that
acts like it does.

Patch attached.

-- 
Robert Haas
EDB: http://www.enterprisedb.com