Re: Fix detection of truncated zstd-compressed backups

Greetings of the day everyone,

Thanks for the detailed review Mr. Daniel and also for the additional
reproducer and suggestions.

I'm a bit busy today, so I'll go through the attached patch and reproducer
carefully tomorrow and work through the review comments.

I'll get back to you once I've had a chance to look through everything.

Regards,
Osama Abdul Qader

On Sat, Sep 5, 2026 at 12:59 AM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:

> > On 14 Aug 2026, at 06:28, Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:
>
> Sorry for the long delay in responding.
>
> > I just checked pg_dump/pg_restore. The problem exists only with zstd and
> lz4, gzip doesn't have the problem.
> >
> > Daniel’s PoC covers the custom-archive-format path, but not the
> directory-format path.
>
> Thanks for expanding the fix, I ran out of time after realizing there was
> an
> issue through my hack.
>
> > For the directory-format path, we can reproduce the problem by simply
> truncating one byte from a compressed data file. For the custom-archive
> path, reproducing the problem is less straightforward because the
> compressed data is stored inside length-prefixed archive blocks. Simply
> truncating the file can make archive parsing fail before the decompressor
> sees the truncated frame. I created a repro script, see the attached shell
> script.
>
> This reasoning should be added as a comment in the test file to aid future
> readers.
>
> > pg_restore: error: could not uncompress data: (null)
>
> While not strictly related we should take the opportunity to fix this
> while in
> here. Perhaps use "unkown error" in case zp->msg is null when erroring
> out?
>
> > While testing, I also found a small issue in LZ4Stream_read_internal().
> Its error branches call pg_log_error() and then return -1, but callers
> immediately call pg_fatal() when the return value <0. This results in
> duplicate error messages. So, I removed those pg_log_error() calls.
>
> Makes sense. The gets function does however not exit with pg_fatal, do we
> need
> any special handling there?
>
> > See 0002 for the fix. I added tests only for zstd and lz4, since gzip is
> not changed.
>
> A few comments on the patches:
>
> +typedef enum
> +{
> + ASTREAMER_STREAM_NEW,
> + ASTREAMER_FRAME_INCOMPLETE,
> + ASTREAMER_FRAME_COMPLETE,
> +} astreamer_decompression_state;
> This needs better commenting. Perhaps something along the lines of:
>
> -/* State of the most recently processed compressed frame. */
> +/*
> + * State of the most recently processed compressed frame. When
> decompression
> + * requires more input data to complete, or a bigger output buffer to
> store
> + * the result the state is set to ASTREAMER_FRAME_INCOMPLETE. Exactly
> how to
> + * resolve an _INCOMPLETE state is compression library dependent. Before
> a
> + * stream has decompressed any frames is has the state
> ASTREAMER_STREAM_NEW.
> + */
>
>
> + bool frame_finished;
> Nitpick: I'm not a fan of using a local variable with the same name (and
> function) as a struct member. Maybe also a comment explaining why we're
> not
> pulling out an LZ4State from the private member?
>
>
> +truncate_custom_compressed_data
> This function needs comments to explain why it's necessary and what it's
> doing.
>
>
> + $pos = index($data, $magic);
> + die "compressed frame magic not found in $path" if $pos < 5;
> + $pos -= 5;
> If the function takes the magic as a parameter it should not make any
> assumptions about the length of the magic. This should either be inferred
> from
> the parameter (best option IMHO) or passed in separately.
>
> +SKIP:
> +{
> + skip "zstd compression not supported by this build", 1 if
> !$supports_zstd;
> Please add a comment explaining why this testcase isn't applicable to LZ4.
>
> --
> Daniel Gustafsson
>
>