Re: Fix detection of truncated zstd-compressed backups

Hi Chao,

I went through the LZ4 part of the v9 patch and tested the frame-completion
handling in the pg_restore read path.

First, I checked how frame_finished is maintained in compress_lz4.c. It is
updated after every LZ4F_decompress() call using the returned status:

state->frame_finished = (status == 0);

I then looked at the LZ4 frame semantics and tested concatenated frames
with the LZ4 command-line tool:

- two complete frames: F1 + F2
- complete first frame followed by a truncated second frame: F1 +
truncated F2

The complete two-frame stream was accepted, while the stream with the
truncated second frame was reported as an unfinished stream.

I then traced the corresponding v9 pg_restore path. In particular,
_PrintFileData() repeatedly calls CFH->read_func() until it reaches EOF or
an error, and the LZ4 read path continues processing buffered compressed
data when bufnext < bufdata. Therefore, after a complete first frame
sets frame_finished
= true, a subsequent incomplete second frame is still passed to
LZ4F_decompress(). Its positive return value changes frame_finished back to
false, and physical EOF subsequently causes the read to fail with EIO.

I also checked the LZ4Stream_close() path. Although it only checks
frame_finished, the normal directory-format pg_restore path does not simply
stop after the first completed frame; it continues consuming the available
compressed data until EOF/error before closing the handle.

Based on these tests and the source-level tracing, the v9 patch correctly
detects the reported truncated LZ4 backup case in the normal pg_restore
directory-format read path.

I did not identify an issue with the LZ4 frame-completion handling in the
path I examined.

With Regards,
Osama Abdul Qader

On Mon, Sep 7, 2026 at 8:43 AM Chao Li <li(dot)evan(dot)chao(at)gmail(dot)com> wrote:

>
>
> > On Sep 5, 2026, at 03:29, 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.
>
> Added a comment in 0002.
>
> >
> >> 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?
>
> Fixed by replacing (null) with “unknown error” in 0002.
>
> >
> >> 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?
>
> Good catch. Fixed in 0002 by moving the error logging from
> LZ4Stream_read_internal() to LZ4Stream_gets().
>
> >
> >> 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.
> > + */
> >
>
> Integrated the suggested comment in 0001.
>
> >
> > + 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?
> >
>
> Renamed the local variable to dec_done that has the naming style as the
> other local variable dec_opts. Also added a comment.
>
> >
> > +truncate_custom_compressed_data
> > This function needs comments to explain why it's necessary and what it's
> doing.
> >
>
> Added a comment.
>
> >
> > + $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.
>
> Replaced the magic 4 and 5 to local variables and added comments to
> explain them.
>
> >
> > +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.
> >
>
> Added a comment.
>
> PFA v9.
>
> Best regards,
> --
> Chao Li (Evan)
> HighGo Software Co., Ltd.
> https://www.highgo.com/
>
>
>
>
>