basebackup/lz4 crash
Forking: <20220316151253.GB28503@telsasoft.com>
On Wed, Mar 16, 2022 at 10:12:54AM -0500, Justin Pryzby wrote:
Also, with a partial regression DB, this crashes when writing to stdout.
$ src/bin/pg_basebackup/pg_basebackup --wal-method fetch -Ft -D - -h /tmp --no-sync --compress=lz4 |wc -c
pg_basebackup: bbstreamer_lz4.c:172: bbstreamer_lz4_compressor_content: Assertion `mystreamer->base.bbs_buffer.maxlen >= out_bound' failed.
24117248#4 0x000055555555e8b4 in bbstreamer_lz4_compressor_content (streamer=0x5555555a5260, member=0x7fffffffc760,
data=0x7ffff3068010 "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n\"Files\": [\n{ \"Path\": \"backup_label\", \"Size\": 227, \"Last-Modified\": \"2022-03-16 02:29:11 GMT\", \"Checksum-Algorithm\": \"CRC32C\", \"Checksum\": \"46f69d99\" },\n{ \"Pa"..., len=401072, context=BBSTREAMER_MEMBER_CONTENTS) at bbstreamer_lz4.c:172
mystreamer = 0x5555555a5260
next_in = 0x7ffff3068010 "{ \"PostgreSQL-Backup-Manifest-Version\": 1,\n\"Files\": [\n{ \"Path\": \"backup_label\", \"Size\": 227, \"Last-Modified\": \"2022-03-16 02:29:11 GMT\", \"Checksum-Algorithm\": \"CRC32C\", \"Checksum\": \"46f69d99\" },\n{ \"Pa"...
...(gdb) p mystreamer->base.bbs_buffer.maxlen
$1 = 524288
(gdb) p (int) LZ4F_compressBound(len, &mystreamer->prefs)
$4 = 524300This is with: liblz4-1:amd64 1.9.2-2ubuntu0.20.04.1
It looks like maybe this code was copied from
bbstreamer_lz4_compressor_finalize() which has an Assert rather than expanding
the buffer like in bbstreamer_lz4_compressor_new()
commit e70c12214b5ba0bc93c083fdb046304a633018ef
Author: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Mon Mar 28 23:24:15 2022 -0500
basebackup: fix crash with lz4 + stdout + manifests
That's just one known way to trigger this issue.
diff --git a/src/bin/pg_basebackup/bbstreamer_lz4.c b/src/bin/pg_basebackup/bbstreamer_lz4.c
index 67f841d96a9..8e8352a450c 100644
--- a/src/bin/pg_basebackup/bbstreamer_lz4.c
+++ b/src/bin/pg_basebackup/bbstreamer_lz4.c
@@ -170,7 +170,11 @@ bbstreamer_lz4_compressor_content(bbstreamer *streamer,
* forward the content to next streamer and empty the buffer.
*/
out_bound = LZ4F_compressBound(len, &mystreamer->prefs);
- Assert(mystreamer->base.bbs_buffer.maxlen >= out_bound);
+
+ /* Enlarge buffer if it falls short of compression bound. */
+ if (mystreamer->base.bbs_buffer.maxlen < out_bound)
+ enlargeStringInfo(&mystreamer->base.bbs_buffer, out_bound);
+
if (avail_out < out_bound)
{
bbstreamer_content(mystreamer->base.bbs_next, member,
On Wed, Mar 30, 2022 at 10:35 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
It looks like maybe this code was copied from
bbstreamer_lz4_compressor_finalize() which has an Assert rather than expanding
the buffer like in bbstreamer_lz4_compressor_new()
Hmm, this isn't great. On the server side, we set up a chain of bbsink
buffers that can't be resized later. Each bbsink tells the next bbsink
how to make its buffer, but the successor bbsink has control of that
buffer and resizing it on-the-fly is not allowed. It looks like
bbstreamer_lz4_compressor_new() is mimicking that logic, but not well.
It sets the initial buffer size to
LZ4F_compressBound(streamer->base.bbs_buffer.maxlen, prefs), but
streamer->base.bbs_buffer.maxlen is not any kind of promise from the
caller about future chunk sizes. It's just whatever initStringInfo()
happens to do. My guess is that Dipesh thought that the buffer
wouldn't need to be resized because "we made it big enough already"
but that's not the case. The server knows how much data it is going to
read from disk at a time, but the client has to deal with whatever the
server sends.
I think your proposed change is OK, modulo some comments. But I think
maybe we ought to delete all the stuff related to compressed_bound
from bbstreamer_lz4_compressor_new() as well, because I don't see that
there's any point. And then I think we should also add logic similar
to what you've added here to bbstreamer_lz4_compressor_finalize(), so
that we're not making the assumption that the buffer will get enlarged
at some earlier point.
Thoughts?
--
Robert Haas
EDB: http://www.enterprisedb.com
Hi,
I think your proposed change is OK, modulo some comments. But I think
maybe we ought to delete all the stuff related to compressed_bound
from bbstreamer_lz4_compressor_new() as well, because I don't see that
there's any point. And then I think we should also add logic similar
to what you've added here to bbstreamer_lz4_compressor_finalize(), so
that we're not making the assumption that the buffer will get enlarged
at some earlier point.Thoughts?
I agree that we should remove the compression bound stuff from
bbstreamer_lz4_compressor_new() and add a fix in
bbstreamer_lz4_compressor_content() and bbstreamer_lz4_compressor_finalize()
to enlarge the buffer if it falls short of the compress bound.
Patch attached.
Thanks,
Dipesh
Attachments:
0001-fix-crash-with-lz4.patchtext/x-patch; charset=US-ASCII; name=0001-fix-crash-with-lz4.patchDownload
From ee9b5e4739bf78b39dc34c9fcc76fc731b09b788 Mon Sep 17 00:00:00 2001
From: Dipesh Pandit <dipesh.pandit@enterprisedb.com>
Date: Thu, 31 Mar 2022 12:19:31 +0530
Subject: [PATCH] fix crash with lz4
We cannot fix the size of output buffer at client during lz4
compression. Client receives chunks from the server and the size of
these chunks varies depending upon the data sent by the server. The
output buffer capacity at client should be adjusted based on the size
of the chunk received from the server.
---
src/bin/pg_basebackup/bbstreamer_lz4.c | 22 ++++++++--------------
1 file changed, 8 insertions(+), 14 deletions(-)
diff --git a/src/bin/pg_basebackup/bbstreamer_lz4.c b/src/bin/pg_basebackup/bbstreamer_lz4.c
index 67f841d..2ffe224 100644
--- a/src/bin/pg_basebackup/bbstreamer_lz4.c
+++ b/src/bin/pg_basebackup/bbstreamer_lz4.c
@@ -73,7 +73,6 @@ bbstreamer_lz4_compressor_new(bbstreamer *next, bc_specification *compress)
bbstreamer_lz4_frame *streamer;
LZ4F_errorCode_t ctxError;
LZ4F_preferences_t *prefs;
- size_t compressed_bound;
Assert(next != NULL);
@@ -92,17 +91,6 @@ bbstreamer_lz4_compressor_new(bbstreamer *next, bc_specification *compress)
if ((compress->options & BACKUP_COMPRESSION_OPTION_LEVEL) != 0)
prefs->compressionLevel = compress->level;
- /*
- * Find out the compression bound, it specifies the minimum destination
- * capacity required in worst case for the success of compression operation
- * (LZ4F_compressUpdate) based on a given source size and preferences.
- */
- compressed_bound = LZ4F_compressBound(streamer->base.bbs_buffer.maxlen, prefs);
-
- /* Enlarge buffer if it falls short of compression bound. */
- if (streamer->base.bbs_buffer.maxlen < compressed_bound)
- enlargeStringInfo(&streamer->base.bbs_buffer, compressed_bound);
-
ctxError = LZ4F_createCompressionContext(&streamer->cctx, LZ4F_VERSION);
if (LZ4F_isError(ctxError))
pg_log_error("could not create lz4 compression context: %s",
@@ -170,7 +158,6 @@ bbstreamer_lz4_compressor_content(bbstreamer *streamer,
* forward the content to next streamer and empty the buffer.
*/
out_bound = LZ4F_compressBound(len, &mystreamer->prefs);
- Assert(mystreamer->base.bbs_buffer.maxlen >= out_bound);
if (avail_out < out_bound)
{
bbstreamer_content(mystreamer->base.bbs_next, member,
@@ -178,6 +165,10 @@ bbstreamer_lz4_compressor_content(bbstreamer *streamer,
mystreamer->bytes_written,
context);
+ /* Enlarge buffer if it falls short of out bound. */
+ if (mystreamer->base.bbs_buffer.maxlen < out_bound)
+ enlargeStringInfo(&mystreamer->base.bbs_buffer, out_bound);
+
avail_out = mystreamer->base.bbs_buffer.maxlen;
mystreamer->bytes_written = 0;
next_out = (uint8 *) mystreamer->base.bbs_buffer.data;
@@ -218,7 +209,6 @@ bbstreamer_lz4_compressor_finalize(bbstreamer *streamer)
/* Find out the footer bound and update the output buffer. */
footer_bound = LZ4F_compressBound(0, &mystreamer->prefs);
- Assert(mystreamer->base.bbs_buffer.maxlen >= footer_bound);
if ((mystreamer->base.bbs_buffer.maxlen - mystreamer->bytes_written) <
footer_bound)
{
@@ -227,6 +217,10 @@ bbstreamer_lz4_compressor_finalize(bbstreamer *streamer)
mystreamer->bytes_written,
BBSTREAMER_UNKNOWN);
+ /* Enlarge buffer if it falls short of footer bound. */
+ if (mystreamer->base.bbs_buffer.maxlen < footer_bound)
+ enlargeStringInfo(&mystreamer->base.bbs_buffer, footer_bound);
+
avail_out = mystreamer->base.bbs_buffer.maxlen;
mystreamer->bytes_written = 0;
next_out = (uint8 *) mystreamer->base.bbs_buffer.data;
--
1.8.3.1
On Thu, Mar 31, 2022 at 3:19 AM Dipesh Pandit <dipesh.pandit@gmail.com> wrote:
Patch attached.
Committed. Thanks.
--
Robert Haas
EDB: http://www.enterprisedb.com