From 00d9dcf5bebbb355152a60f0e2120cdf7f9e7ddd Mon Sep 17 00:00:00 2001 From: Tomas Vondra Date: Wed, 29 Mar 2023 00:50:34 +0200 Subject: [PATCH] pg_dump: Fix gzip compression of empty data The pg_dump Compressor API has three basic callbacks - Allocate, Write and End. The gzip implementation (since e9960732a) wrongly assumed the Write function would always be called, and deferred the initialization of the internal compression system until the first such call. But when there's no data to compress (e.g. for empty LO), this would result in not finalizing the compression state (because it was not actually initialized), producing invalid dump. Fixed by initializing the internal compression system in the Allocate call, whenever the caller provides the Write. For decompression the state is not needed, so we leave the private_data member unpopulated. Introduces a pg_dump TAP test compressing an empty large object. This also rearranges the functions to their original order, to make diffs against older code simpler to understand. Finally, replace an unreachable pg_fatal() with a simple assert check. Reported-by: Justin Pryzby Author: Justin Pryzby, Georgios Kokolatos Reviewed-by: Georgios Kokolatos, Tomas Vondra https://postgr.es/m/20230228235834.GC30529%40telsasoft.com --- src/bin/pg_dump/compress_gzip.c | 136 +++++++++++++++++-------------- src/bin/pg_dump/t/002_pg_dump.pl | 23 ++++++ 2 files changed, 100 insertions(+), 59 deletions(-) diff --git a/src/bin/pg_dump/compress_gzip.c b/src/bin/pg_dump/compress_gzip.c index cec0b41fce..63dfd9668c 100644 --- a/src/bin/pg_dump/compress_gzip.c +++ b/src/bin/pg_dump/compress_gzip.c @@ -33,8 +33,73 @@ typedef struct GzipCompressorState } GzipCompressorState; /* Private routines that support gzip compressed data I/O */ +static void DeflateCompressorInit(CompressorState *cs); +static void DeflateCompressorEnd(ArchiveHandle *AH, CompressorState *cs); +static void DeflateCompressorCommon(ArchiveHandle *AH, CompressorState *cs, + bool flush); +static void EndCompressorGzip(ArchiveHandle *AH, CompressorState *cs); +static void WriteDataToArchiveGzip(ArchiveHandle *AH, CompressorState *cs, + const void *data, size_t dLen); +static void ReadDataFromArchiveGzip(ArchiveHandle *AH, CompressorState *cs); + static void -DeflateCompressorGzip(ArchiveHandle *AH, CompressorState *cs, bool flush) +DeflateCompressorInit(CompressorState *cs) +{ + GzipCompressorState *gzipcs; + z_streamp zp; + + gzipcs = (GzipCompressorState *) pg_malloc0(sizeof(GzipCompressorState)); + zp = gzipcs->zp = (z_streamp) pg_malloc(sizeof(z_stream)); + zp->zalloc = Z_NULL; + zp->zfree = Z_NULL; + zp->opaque = Z_NULL; + + /* + * outsize is the buffer size we tell zlib it can output to. We actually + * allocate one extra byte because some routines want to append a trailing + * zero byte to the zlib output. + */ + gzipcs->outsize = DEFAULT_IO_BUFFER_SIZE; + gzipcs->outbuf = pg_malloc(gzipcs->outsize + 1); + + /* -Z 0 uses the "None" compressor -- not zlib with no compression */ + Assert(cs->compression_spec.level != 0); + + if (deflateInit(zp, cs->compression_spec.level) != Z_OK) + pg_fatal("could not initialize compression library: %s", zp->msg); + + /* Just be paranoid - maybe End is called after Start, with no Write */ + zp->next_out = gzipcs->outbuf; + zp->avail_out = gzipcs->outsize; + + /* Keep track of gzipcs */ + cs->private_data = gzipcs; +} + +static void +DeflateCompressorEnd(ArchiveHandle *AH, CompressorState *cs) +{ + GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data; + z_streamp zp; + + zp = gzipcs->zp; + zp->next_in = NULL; + zp->avail_in = 0; + + /* Flush any remaining data from zlib buffer */ + DeflateCompressorCommon(AH, cs, true); + + if (deflateEnd(zp) != Z_OK) + pg_fatal("could not close compression stream: %s", zp->msg); + + pg_free(gzipcs->outbuf); + pg_free(gzipcs->zp); + pg_free(gzipcs); + cs->private_data = NULL; +} + +static void +DeflateCompressorCommon(ArchiveHandle *AH, CompressorState *cs, bool flush) { GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data; z_streamp zp = gzipcs->zp; @@ -78,27 +143,9 @@ DeflateCompressorGzip(ArchiveHandle *AH, CompressorState *cs, bool flush) static void EndCompressorGzip(ArchiveHandle *AH, CompressorState *cs) { - GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data; - z_streamp zp; - - if (gzipcs->zp) - { - zp = gzipcs->zp; - zp->next_in = NULL; - zp->avail_in = 0; - - /* Flush any remaining data from zlib buffer */ - DeflateCompressorGzip(AH, cs, true); - - if (deflateEnd(zp) != Z_OK) - pg_fatal("could not close compression stream: %s", zp->msg); - - pg_free(gzipcs->outbuf); - pg_free(gzipcs->zp); - } - - pg_free(gzipcs); - cs->private_data = NULL; + /* If deflation was initialized, finalize it */ + if (cs->private_data) + DeflateCompressorEnd(AH, cs); } static void @@ -106,41 +153,10 @@ WriteDataToArchiveGzip(ArchiveHandle *AH, CompressorState *cs, const void *data, size_t dLen) { GzipCompressorState *gzipcs = (GzipCompressorState *) cs->private_data; - z_streamp zp; - - if (!gzipcs->zp) - { - zp = gzipcs->zp = (z_streamp) pg_malloc(sizeof(z_stream)); - zp->zalloc = Z_NULL; - zp->zfree = Z_NULL; - zp->opaque = Z_NULL; - - /* - * outsize is the buffer size we tell zlib it can output to. We - * actually allocate one extra byte because some routines want to - * append a trailing zero byte to the zlib output. - */ - gzipcs->outsize = DEFAULT_IO_BUFFER_SIZE; - gzipcs->outbuf = pg_malloc(gzipcs->outsize + 1); - - /* - * A level of zero simply copies the input one block at the time. This - * is probably not what the user wanted when calling this interface. - */ - if (cs->compression_spec.level == 0) - pg_fatal("requested to compress the archive yet no level was specified"); - - if (deflateInit(zp, cs->compression_spec.level) != Z_OK) - pg_fatal("could not initialize compression library: %s", zp->msg); - - /* Just be paranoid - maybe End is called after Start, with no Write */ - zp->next_out = gzipcs->outbuf; - zp->avail_out = gzipcs->outsize; - } gzipcs->zp->next_in = (void *) unconstify(void *, data); gzipcs->zp->avail_in = dLen; - DeflateCompressorGzip(AH, cs, false); + DeflateCompressorCommon(AH, cs, false); } static void @@ -214,17 +230,19 @@ void InitCompressorGzip(CompressorState *cs, const pg_compress_specification compression_spec) { - GzipCompressorState *gzipcs; - cs->readData = ReadDataFromArchiveGzip; cs->writeData = WriteDataToArchiveGzip; cs->end = EndCompressorGzip; cs->compression_spec = compression_spec; - gzipcs = (GzipCompressorState *) pg_malloc0(sizeof(GzipCompressorState)); - - cs->private_data = gzipcs; + /* + * If the caller has defined a write function, prepare the necessary + * state. Note that if the data is empty, End may be called immediately + * after Init, without ever calling Write. + */ + if (cs->writeF) + DeflateCompressorInit(cs); } diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl index a22f27f300..42215f82f7 100644 --- a/src/bin/pg_dump/t/002_pg_dump.pl +++ b/src/bin/pg_dump/t/002_pg_dump.pl @@ -1265,6 +1265,29 @@ my %tests = ( }, }, + 'LO create (with no data)' => { + create_sql => + 'SELECT pg_catalog.lo_create(0);', + regexp => qr/^ + \QSELECT pg_catalog.lo_open\E \('\d+',\ \d+\);\n + \QSELECT pg_catalog.lo_close(0);\E + /xm, + like => { + %full_runs, + column_inserts => 1, + data_only => 1, + inserts => 1, + section_data => 1, + test_schema_plus_large_objects => 1, + }, + unlike => { + binary_upgrade => 1, + no_large_objects => 1, + schema_only => 1, + section_pre_data => 1, + }, + }, + 'COMMENT ON DATABASE postgres' => { regexp => qr/^COMMENT ON DATABASE postgres IS .+;/m,