Decompression bug in astreamer_lz4

Started by Mikhail Gribkov7 months ago4 messages
#1Mikhail Gribkov
youzhick@gmail.com
1 attachment(s)

Hi hackers,

While developing my extension, I faced some problems in using
pg_basebackup/pg_verifybackup on a cluster with additional custom files
saved. The extension saves its own format files, which may have a variety
of sizes and usually quite low compression potential.

The same situation can be reproduced using randomly filled files. Let’s
imagine the extension saves its own statistics in pg_stat:
dd if=/dev/urandom of=$PGDATA/pg_stat/junk bs=1M count=16

Now we’ll run the server and make an lz4-packed backup:
$PGINSTALL/bin/pg_basebackup --no-sync -cfast --target
server:/tmp/server-backup -Xfetch --compress server-lz4

Now we’ll check the backup:
$PGINSTALL/bin/pg_verifybackup -n /tmp/server-backup

The verification process will fail. The exact error may vary, but the most
common version looks like this:
pg_verifybackup: error: checksum mismatch for file "pg_stat/junk" in
archive "base.tar.lz4"
pg_verifybackup: error: "base/4/3431" is present in the manifest but not on
disk
pg_verifybackup: error: "base/5/2620" is present in the manifest but not on
disk
pg_verifybackup: error: "base/1/2617" is present in the manifest but not on
disk
pg_verifybackup: error: "base/4/3576" is present in the manifest but not on
disk
......

Using gzip or zstd compressions in the same setup doesn’t lead to any
problems. Moreover, the lz4 archive itself is fine and it can be
successfully unpacked via cli lz4 unpacker. The problem thus is in reading,
and some investigation done by me and my colleagues lead us to
astreamer_lz4.c file.
Looks like astreamer_lz4_decompressor_content() function is initializing
_out-parameters incorrectly:
next_out = (uint8 *) mystreamer->base.bbs_buffer.data;
avail_out = mystreamer->base.bbs_buffer.maxlen;

, while should be:
next_out = (uint8 *) mystreamer->base.bbs_buffer.data +
mystreamer->bytes_written;
avail_out = mystreamer->base.bbs_buffer.maxlen - mystreamer->bytes_written;

And this would be consistent with what the gzip version of the function
(astreamer_gzip_decompressor_content()) does.

In the vast majority of cases the mystreamer->bytes_written value is zero
at the moment of next_out/avail_out initialization, thus the problem does
not appear. In fact we were totally unable to reproduce non-zero initial
value of mystreamer->bytes_written without poor-compressible custom format
files.I’m not that deep in the compression algorithms to say exactly why is
that, and I realize that very few users will really face the problem, but
the problem is definitely there.

Attached is a simple patch implementing the above fix. The issue disappears
with it.
What do you think?

--
best regards,
Mikhail A. Gribkov

Attachments:

v1-Fix-lz4-decompressor.patchapplication/octet-stream; name=v1-Fix-lz4-decompressor.patchDownload
diff --git a/src/fe_utils/astreamer_lz4.c b/src/fe_utils/astreamer_lz4.c
index 781aaf99f3..5f581d1de3 100644
--- a/src/fe_utils/astreamer_lz4.c
+++ b/src/fe_utils/astreamer_lz4.c
@@ -322,9 +322,9 @@ astreamer_lz4_decompressor_content(astreamer *streamer,
 
 	mystreamer = (astreamer_lz4_frame *) streamer;
 	next_in = (uint8 *) data;
-	next_out = (uint8 *) mystreamer->base.bbs_buffer.data;
+	next_out = (uint8 *) mystreamer->base.bbs_buffer.data + mystreamer->bytes_written;
 	avail_in = len;
-	avail_out = mystreamer->base.bbs_buffer.maxlen;
+	avail_out = mystreamer->base.bbs_buffer.maxlen - mystreamer->bytes_written;
 
 	while (avail_in > 0)
 	{
#2Michael Paquier
michael@paquier.xyz
In reply to: Mikhail Gribkov (#1)
Re: Decompression bug in astreamer_lz4

On Wed, Jun 25, 2025 at 11:07:32PM +0300, Mikhail Gribkov wrote:

The same situation can be reproduced using randomly filled files. Let’s
imagine the extension saves its own statistics in pg_stat:
dd if=/dev/urandom of=$PGDATA/pg_stat/junk bs=1M count=16

Original, I like that. It's not hard to imagine some extension code
that decides to dump at the root of the data folder some file using
one of the shutdown callbacks at the root of the data directory. Not
seeing the problem would then be a matter of luck. So yes, that's a
legit bug IMO.

In the vast majority of cases the mystreamer->bytes_written value is zero
at the moment of next_out/avail_out initialization, thus the problem does
not appear. In fact we were totally unable to reproduce non-zero initial
value of mystreamer->bytes_written without poor-compressible custom format
files.I’m not that deep in the compression algorithms to say exactly why is
that, and I realize that very few users will really face the problem, but
the problem is definitely there.

That's not surprising, noted.

Attached is a simple patch implementing the above fix. The issue disappears
with it.

It seems like you have spent some time digging into all that, thanks
for all that.

What do you think?

I think that it would be nice to have some test coverage for such
cases in pg_verifybackup for both the client side and the server side
of backups compressed, relying on the backend to generate some random
data dumped into a file at the root of a data folder. You could just
plug in that into one of the scripts, before taking a backup, and
check all the compression methods we need to support. See for example
brin.sql where with the combination of string_agg() and fipshash(), as
one reference. So we could just reuse something like that.

I am detecting a threshold close to 512kB to be able to reproduce the
problem, so we don't need a file that large to see the failure, making
such tests faster with less data required as long as the file is
written once in a data folder used by all the backups taken:
$ rm $PGDATA/junk && dd if=/dev/urandom of=$PGDATA/junk bs=1k count=512
$ rm -r /tmp/client-backup-lz4/*
$ pg_basebackup --format=tar --no-sync -cfast \
-Xfetch --compress client-lz4 -D /tmp/client-backup-lz4
$ pg_verifybackup -n /tmp/client-backup-lz4/
pg_verifybackup: error: checksum mismatch for file "junk" in archive "base.tar.lz4"
--
Michael

#3Mikhail Gribkov
youzhick@gmail.com
In reply to: Michael Paquier (#2)
1 attachment(s)
Re: Decompression bug in astreamer_lz4

Hi Michael,

I think that it would be nice to have some test coverage for such
cases in pg_verifybackup for both the client side and the server side
of backups compressed, relying on the backend to generate some random
data dumped into a file at the root of a data folder.

Yes, good point. Added checks to both client and server untar tests.
I also noticed that the problem showup depends on compression level. For
example in 008_untar.pl and 010_client_untar.pl tests it only showed up
with compression levels 0 and 1.
It looks to me that we do not have any chances to check all possible
combinations of vital parameters, but at least we'll check several cases
that we know make a difference.

I am detecting a threshold close to 512kB to be able to reproduce the
problem, so we don't need a file that large to see the failure, making
such tests faster with less data required

I have raised the file size up to 640kB to make it a bit more reliable as
far as we are not sure about all lz4-internals that trigger the problem. It
still does not make the test data unreasonably large.

Attached is the updated version of the patch with test additions.

--
best regards,
Mikhail A. Gribkov

Show quoted text

Attachments:

v2-Fix-lz4-decompressor.patchapplication/octet-stream; name=v2-Fix-lz4-decompressor.patchDownload
diff --git a/src/bin/pg_verifybackup/t/008_untar.pl b/src/bin/pg_verifybackup/t/008_untar.pl
index deed3ec247..c2e0e2f3a6 100644
--- a/src/bin/pg_verifybackup/t/008_untar.pl
+++ b/src/bin/pg_verifybackup/t/008_untar.pl
@@ -16,6 +16,24 @@ my $primary = PostgreSQL::Test::Cluster->new('primary');
 $primary->init(allows_streaming => 1);
 $primary->start;
 
+# Create randomly filled file at the root of the data folder to check possible
+# decompression issues. There was a bug reported, that didn't show up with
+# a regular data folder contents. The bug itself was only detected for lz4
+# decompression with levels 0 and 1, and a minimal file size about 512k. It
+# doesn't seem worthy checking too wide variety of combinations, but at least
+# we'll add this file for all compression methods checks.
+# (https://www.postgresql.org/message-id/CAMEv5_uQS1Hg6KCaEP2JkrTBbZ-nXQhxomWrhYQvbdzR-zy-wA%40mail.gmail.com)
+my $junk_data = $primary->safe_psql(
+	'postgres', qq(
+		SELECT string_agg(encode(sha256(i::bytea), 'hex'), '')
+		FROM generate_series(1, 10240) s(i);));
+my $data_dir = $primary->data_dir;
+my $junk_file = "$data_dir/junk";
+open my $jf, '>', $junk_file
+	or die "Could not create junk file: $!";
+print $jf $junk_data;
+close $jf;
+
 # Create a tablespace directory.
 my $source_ts_path = PostgreSQL::Test::Utils::tempdir_short();
 
@@ -52,6 +70,12 @@ my @test_configuration = (
 		'backup_archive' => [ 'base.tar.lz4', "$tsoid.tar.lz4" ],
 		'enabled' => check_pg_config("#define USE_LZ4 1")
 	},
+	{
+		'compression_method' => 'lz4',
+		'backup_flags' => [ '--compress', 'server-lz4:5' ],
+		'backup_archive' => [ 'base.tar.lz4', "$tsoid.tar.lz4" ],
+		'enabled' => check_pg_config("#define USE_LZ4 1")
+	},
 	{
 		'compression_method' => 'zstd',
 		'backup_flags' => [ '--compress', 'server-zstd' ],
@@ -111,5 +135,4 @@ for my $tc (@test_configuration)
 		rmtree($extract_path);
 	}
 }
-
 done_testing();
diff --git a/src/bin/pg_verifybackup/t/010_client_untar.pl b/src/bin/pg_verifybackup/t/010_client_untar.pl
index d8d2b06c7e..d0d7dfb4af 100644
--- a/src/bin/pg_verifybackup/t/010_client_untar.pl
+++ b/src/bin/pg_verifybackup/t/010_client_untar.pl
@@ -15,6 +15,24 @@ my $primary = PostgreSQL::Test::Cluster->new('primary');
 $primary->init(allows_streaming => 1);
 $primary->start;
 
+# Create randomly filled file at the root of the data folder to check possible
+# decompression issues. There was a bug reported, that didn't show up with
+# a regular data folder contents. The bug itself was only detected for lz4
+# decompression with levels 0 and 1, and a minimal file size about 512k. It
+# doesn't seem worthy checking too wide variety of combinations, but at least
+# we'll add this file for all compression methods checks.
+# (https://www.postgresql.org/message-id/CAMEv5_uQS1Hg6KCaEP2JkrTBbZ-nXQhxomWrhYQvbdzR-zy-wA%40mail.gmail.com)
+my $junk_data = $primary->safe_psql(
+	'postgres', qq(
+		SELECT string_agg(encode(sha256(i::bytea), 'hex'), '')
+		FROM generate_series(1, 10240) s(i);));
+my $data_dir = $primary->data_dir;
+my $junk_file = "$data_dir/junk";
+open my $jf, '>', $junk_file
+	or die "Could not create junk file: $!";
+print $jf $junk_data;
+close $jf;
+
 my $backup_path = $primary->backup_dir . '/client-backup';
 my $extract_path = $primary->backup_dir . '/extracted-backup';
 
@@ -37,6 +55,12 @@ my @test_configuration = (
 		'backup_archive' => 'base.tar.lz4',
 		'enabled' => check_pg_config("#define USE_LZ4 1")
 	},
+	{
+		'compression_method' => 'lz4',
+		'backup_flags' => [ '--compress', 'client-lz4:1' ],
+		'backup_archive' => 'base.tar.lz4',
+		'enabled' => check_pg_config("#define USE_LZ4 1")
+	},
 	{
 		'compression_method' => 'zstd',
 		'backup_flags' => [ '--compress', 'client-zstd:5' ],
@@ -125,5 +149,4 @@ for my $tc (@test_configuration)
 		rmtree($backup_path);
 	}
 }
-
 done_testing();
diff --git a/src/fe_utils/astreamer_lz4.c b/src/fe_utils/astreamer_lz4.c
index 781aaf99f3..5f581d1de3 100644
--- a/src/fe_utils/astreamer_lz4.c
+++ b/src/fe_utils/astreamer_lz4.c
@@ -322,9 +322,9 @@ astreamer_lz4_decompressor_content(astreamer *streamer,
 
 	mystreamer = (astreamer_lz4_frame *) streamer;
 	next_in = (uint8 *) data;
-	next_out = (uint8 *) mystreamer->base.bbs_buffer.data;
+	next_out = (uint8 *) mystreamer->base.bbs_buffer.data + mystreamer->bytes_written;
 	avail_in = len;
-	avail_out = mystreamer->base.bbs_buffer.maxlen;
+	avail_out = mystreamer->base.bbs_buffer.maxlen - mystreamer->bytes_written;
 
 	while (avail_in > 0)
 	{
#4Michael Paquier
michael@paquier.xyz
In reply to: Mikhail Gribkov (#3)
Re: Decompression bug in astreamer_lz4

On Fri, Jun 27, 2025 at 03:28:18PM +0300, Mikhail Gribkov wrote:

Attached is the updated version of the patch with test additions.

Thanks for the updated versions. A bit more variance with the LZ4
levels, even if we cannot track everything, does not sound like a bad
thing to me for the server-side and client-side [de]compressions. I
have reused what you have suggested, adjusting the tests in the
back-branches based on the fact that int->bytea casts don't exist up
to v17 and that the command structures were a bit different in test
008.

And fixed down to v15.
--
Michael