Re: pg_verifybackup: TAR format backup verification

On Sun, Sep 29, 2024 at 1:03 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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