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

  1. Adjust json_manifest_per_file_callback API in one more place.

  2. File size in a backup manifest should use uint64, not size_t.

  3. Fix some pg_verifybackup issues reported by Coverity.

  4. pg_verifybackup: Move some declarations to new pg_verifybackup.h

  5. pg_verifybackup: Move skip_checksums into verifier_context.

  6. Improve file header comments for astramer code.

  7. Move astreamer (except astreamer_inject) to fe_utils.

  8. Move recovery injector astreamer to a separate header file.

  9. Rename bbstreamer to astreamer.