Horton

Re: pg_verifybackup: TAR format backup verification

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
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-30T15:31:01Z
Lists: pgsql-hackers 
Robert Haas <robertmhaas@gmail.com> writes:
> On Sun, Sep 29, 2024 at 1:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> CID 1620458:  Resource leaks  (RESOURCE_LEAK)
>>> Variable "buffer" going out of scope leaks the storage it points to.

> 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.

+1

>>> 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".

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

That's clearly an improvement, but I was wondering if we should also
change "len" and "remaining" to be unsigned (probably size_t).
Coverity might be unhappy about the off-the-end array reference,
but perhaps it's also concerned about what happens if len is negative.

>>> CID 1620456:  Null pointer dereferences  (FORWARD_NULL)
>>> Passing null pointer "suffix" to "strcmp", which dereferences it.

> 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.

Check.

> Patch attached.

WFM, modulo the suggestion about changing data types.

			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.