On Sun, Sep 29, 2024 at 1:03 PM Tom Lane <t...@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
0001-Fix-issues-reported-by-Coverity.patch
Description: Binary data
