Re: pg_verifybackup: TAR format backup verification
Tom Lane <tgl@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Amul Sul <sulamul@gmail.com>, Sravan Kumar <sravanvcybage@gmail.com>,
PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2024-09-29T17:03:38Z
Lists: pgsql-hackers
Piling on a bit ... Coverity reported the following issues in
this new code. I have not analyzed them to see if they're
real problems.
________________________________________________________________________________________________________
*** 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 */
________________________________________________________________________________________________________
*** 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. */
________________________________________________________________________________________________________
*** 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;
regards, tom lane
Commits
-
Adjust json_manifest_per_file_callback API in one more place.
- 554d3a18f362 18.0 landed
-
File size in a backup manifest should use uint64, not size_t.
- d94cf5ca7fad 18.0 landed
-
Fix some pg_verifybackup issues reported by Coverity.
- fc1b2ce0ee9c 18.0 landed
-
pg_verifybackup: Move some declarations to new pg_verifybackup.h
- aa2d6b15d6d6 18.0 landed
-
pg_verifybackup: Move skip_checksums into verifier_context.
- af99d44a889f 18.0 landed
-
Improve file header comments for astramer code.
- 22b4a1b561f8 18.0 landed
-
Move astreamer (except astreamer_inject) to fe_utils.
- f80b09bac87d 18.0 landed
-
Move recovery injector astreamer to a separate header file.
- 53b2c921a0f9 18.0 landed
-
Rename bbstreamer to astreamer.
- 3c905698114d 18.0 landed