From 94ca77e1fbdcf063f9a4f0957c03ef7cf1829cc4 Mon Sep 17 00:00:00 2001 From: Georgios Kokolatos Date: Thu, 16 Mar 2023 16:06:00 +0000 Subject: [PATCH v2 1/3] Improve type handling in pg_dump's compress file API The function LZ4File_gets() was storing the return value of LZ4File_read_internal in a variable of the wrong type, disregarding sign-es. As a consequence, LZ4File_gets() would not take the error path when it should. In an attempt to improve readability and code uniformity, change the return type of the API's read and write functions to integer from size_t. Along with it, homogenize the return values of the relevant functions of this API. This change, helps the specific compression implementations handle the return types of their corresponding libraries internally and not expose them to the API caller. With the help and guidance of Tomas Vondra. Reported-by: Alexander Lakhin --- src/bin/pg_dump/compress_gzip.c | 24 ++++++++++----- src/bin/pg_dump/compress_io.h | 28 ++++++++++++++--- src/bin/pg_dump/compress_lz4.c | 43 ++++++++++++++------------- src/bin/pg_dump/compress_none.c | 19 ++++++++---- src/bin/pg_dump/pg_backup_archiver.c | 3 +- src/bin/pg_dump/pg_backup_directory.c | 14 ++++----- 6 files changed, 85 insertions(+), 46 deletions(-) diff --git a/src/bin/pg_dump/compress_gzip.c b/src/bin/pg_dump/compress_gzip.c index 0af65afeb4..29e2fd8d50 100644 --- a/src/bin/pg_dump/compress_gzip.c +++ b/src/bin/pg_dump/compress_gzip.c @@ -233,14 +233,14 @@ InitCompressorGzip(CompressorState *cs, *---------------------- */ -static size_t -Gzip_read(void *ptr, size_t size, CompressFileHandle *CFH) +static int +Gzip_read(void *ptr, size_t size, size_t *rsize, CompressFileHandle *CFH) { gzFile gzfp = (gzFile) CFH->private_data; - size_t ret; + size_t gzret; - ret = gzread(gzfp, ptr, size); - if (ret != size && !gzeof(gzfp)) + gzret = gzread(gzfp, ptr, size); + if (gzret != size && !gzeof(gzfp)) { int errnum; const char *errmsg = gzerror(gzfp, &errnum); @@ -249,15 +249,23 @@ Gzip_read(void *ptr, size_t size, CompressFileHandle *CFH) errnum == Z_ERRNO ? strerror(errno) : errmsg); } - return ret; + if (rsize) + *rsize = gzret; + + return 0; } -static size_t +static int Gzip_write(const void *ptr, size_t size, CompressFileHandle *CFH) { gzFile gzfp = (gzFile) CFH->private_data; + size_t gzret; + + gzret = gzwrite(gzfp, ptr, size); + if (gzret != size) + return 1; - return gzwrite(gzfp, ptr, size); + return 0; } static int diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h index cdb15951ea..b03d5b325b 100644 --- a/src/bin/pg_dump/compress_io.h +++ b/src/bin/pg_dump/compress_io.h @@ -100,6 +100,8 @@ struct CompressFileHandle * Pass either 'path' or 'fd' depending on whether a file path or a file * descriptor is available. 'mode' can be one of 'r', 'rb', 'w', 'wb', * 'a', and 'ab'. Requires an already initialized CompressFileHandle. + * + * Returns zero on success and non-zero on error. */ int (*open_func) (const char *path, int fd, const char *mode, CompressFileHandle *CFH); @@ -109,19 +111,27 @@ struct CompressFileHandle * * 'mode' can be one of 'w', 'wb', 'a', and 'ab'. Requires an already * initialized CompressFileHandle. + * + * Returns zero on success and non-zero on error. */ int (*open_write_func) (const char *path, const char *mode, CompressFileHandle *CFH); /* * Read 'size' bytes of data from the file and store them into 'ptr'. + * Optionally it will store the number of bytes read in 'rsize'. + * + * Returns zero on success and non-zero on error. */ - size_t (*read_func) (void *ptr, size_t size, CompressFileHandle *CFH); + int (*read_func) (void *ptr, size_t size, size_t *rsize, + CompressFileHandle *CFH); /* * Write 'size' bytes of data into the file from 'ptr'. + * + * Returns zero on success and non-zero on error. */ - size_t (*write_func) (const void *ptr, size_t size, + int (*write_func) (const void *ptr, size_t size, struct CompressFileHandle *CFH); /* @@ -130,28 +140,38 @@ struct CompressFileHandle * * Stop if an EOF or a newline is found first. 's' is always null * terminated and contains the newline if it was found. + * + * Returns 's' on success, and NULL on error or when end of file occurs + * while no characters have been read. */ char *(*gets_func) (char *s, int size, CompressFileHandle *CFH); /* * Read the next character from the compress file handle as 'unsigned * char' cast into 'int'. + * + * Returns the character read on success and throws an internal error + * otherwise. It treats EOF as error. */ int (*getc_func) (CompressFileHandle *CFH); /* * Test if EOF is reached in the compress file handle. + * + * Returns non-zero if it is reached. */ int (*eof_func) (CompressFileHandle *CFH); /* * Close an open file handle. + * + * Returns zero on success and non-zero on error. */ int (*close_func) (CompressFileHandle *CFH); /* - * Get a pointer to a string that describes an error that occurred during a - * compress file handle operation. + * Get a pointer to a string that describes an error that occurred during + * a compress file handle operation. */ const char *(*get_error_func) (CompressFileHandle *CFH); diff --git a/src/bin/pg_dump/compress_lz4.c b/src/bin/pg_dump/compress_lz4.c index 63e794cdc6..df2b4c9546 100644 --- a/src/bin/pg_dump/compress_lz4.c +++ b/src/bin/pg_dump/compress_lz4.c @@ -302,9 +302,9 @@ LZ4File_read_overflow(LZ4File *fs, void *ptr, int size, bool eol_flag) static int LZ4File_read_internal(LZ4File *fs, void *ptr, int ptrsize, bool eol_flag) { - size_t dsize = 0; - size_t rsize; - size_t size = ptrsize; + int dsize = 0; + int rsize; + int size = ptrsize; bool eol_found = false; void *readbuf; @@ -398,17 +398,17 @@ LZ4File_read_internal(LZ4File *fs, void *ptr, int ptrsize, bool eol_flag) fs->overflowlen += outlen; } } - } while (rsize == size && dsize < size && eol_found == 0); + } while (rsize == size && dsize < size && eol_found == false); pg_free(readbuf); - return (int) dsize; + return dsize; } /* * Compress size bytes from ptr and write them to the stream. */ -static size_t +static int LZ4File_write(const void *ptr, size_t size, CompressFileHandle *CFH) { LZ4File *fs = (LZ4File *) CFH->private_data; @@ -417,7 +417,7 @@ LZ4File_write(const void *ptr, size_t size, CompressFileHandle *CFH) /* Lazy init */ if (LZ4File_init(fs, size, true)) - return -1; + return 1; while (remaining > 0) { @@ -430,7 +430,7 @@ LZ4File_write(const void *ptr, size_t size, CompressFileHandle *CFH) if (LZ4F_isError(status)) { fs->errcode = status; - return -1; + return 1; } if (fwrite(fs->buffer, 1, status, fs->fp) != status) @@ -440,23 +440,25 @@ LZ4File_write(const void *ptr, size_t size, CompressFileHandle *CFH) } } - return size; + return 0; } /* * fread() equivalent implementation for LZ4 compressed files. */ -static size_t -LZ4File_read(void *ptr, size_t size, CompressFileHandle *CFH) +static int +LZ4File_read(void *ptr, size_t size, size_t *rsize, CompressFileHandle *CFH) { LZ4File *fs = (LZ4File *) CFH->private_data; int ret; - ret = LZ4File_read_internal(fs, ptr, size, false); - if (ret != size && !LZ4File_eof(CFH)) + if ((ret = LZ4File_read_internal(fs, ptr, size, false)) < 0) pg_fatal("could not read from input file: %s", LZ4File_get_error(CFH)); - return ret; + if (rsize) + *rsize = (size_t) ret; + + return 0; } /* @@ -468,7 +470,7 @@ LZ4File_getc(CompressFileHandle *CFH) LZ4File *fs = (LZ4File *) CFH->private_data; unsigned char c; - if (LZ4File_read_internal(fs, &c, 1, false) != 1) + if (LZ4File_read_internal(fs, &c, 1, false) <= 0) { if (!LZ4File_eof(CFH)) pg_fatal("could not read from input file: %s", LZ4File_get_error(CFH)); @@ -486,14 +488,14 @@ static char * LZ4File_gets(char *ptr, int size, CompressFileHandle *CFH) { LZ4File *fs = (LZ4File *) CFH->private_data; - size_t dsize; + int ret; - dsize = LZ4File_read_internal(fs, ptr, size, true); - if (dsize < 0) + ret = LZ4File_read_internal(fs, ptr, size, true); + if (ret < 0 || (ret == 0 && !LZ4File_eof(CFH))) pg_fatal("could not read from input file: %s", LZ4File_get_error(CFH)); /* Done reading */ - if (dsize == 0) + if (ret == 0) return NULL; return ptr; @@ -509,7 +511,6 @@ LZ4File_close(CompressFileHandle *CFH) FILE *fp; LZ4File *fs = (LZ4File *) CFH->private_data; size_t status; - int ret; fp = fs->fp; if (fs->inited) @@ -520,7 +521,7 @@ LZ4File_close(CompressFileHandle *CFH) if (LZ4F_isError(status)) pg_fatal("failed to end compression: %s", LZ4F_getErrorName(status)); - else if ((ret = fwrite(fs->buffer, 1, status, fs->fp)) != status) + else if (fwrite(fs->buffer, 1, status, fs->fp) != status) { errno = (errno) ? errno : ENOSPC; WRITE_ERROR_EXIT; diff --git a/src/bin/pg_dump/compress_none.c b/src/bin/pg_dump/compress_none.c index ecbcf4b04a..bd479fde59 100644 --- a/src/bin/pg_dump/compress_none.c +++ b/src/bin/pg_dump/compress_none.c @@ -83,8 +83,8 @@ InitCompressorNone(CompressorState *cs, * Private routines */ -static size_t -read_none(void *ptr, size_t size, CompressFileHandle *CFH) +static int +read_none(void *ptr, size_t size, size_t *rsize, CompressFileHandle *CFH) { FILE *fp = (FILE *) CFH->private_data; size_t ret; @@ -97,13 +97,22 @@ read_none(void *ptr, size_t size, CompressFileHandle *CFH) pg_fatal("could not read from input file: %s", strerror(errno)); - return ret; + if (rsize) + *rsize = ret; + + return 0; } -static size_t +static int write_none(const void *ptr, size_t size, CompressFileHandle *CFH) { - return fwrite(ptr, 1, size, (FILE *) CFH->private_data); + size_t ret; + + ret = fwrite(ptr, 1, size, (FILE *) CFH->private_data); + if (ret != size) + return 1; + + return 0; } static const char * diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c index 61ebb8fe85..138ea158f1 100644 --- a/src/bin/pg_dump/pg_backup_archiver.c +++ b/src/bin/pg_dump/pg_backup_archiver.c @@ -1694,7 +1694,8 @@ ahwrite(const void *ptr, size_t size, size_t nmemb, ArchiveHandle *AH) { CompressFileHandle *CFH = (CompressFileHandle *) AH->OF; - bytes_written = CFH->write_func(ptr, size * nmemb, CFH); + if (!CFH->write_func(ptr, size * nmemb, CFH)) + bytes_written = size * nmemb; } if (bytes_written != size * nmemb) diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c index 41c2b733e3..1cd9805ef7 100644 --- a/src/bin/pg_dump/pg_backup_directory.c +++ b/src/bin/pg_dump/pg_backup_directory.c @@ -348,7 +348,7 @@ _WriteData(ArchiveHandle *AH, const void *data, size_t dLen) CompressFileHandle *CFH = ctx->dataFH; errno = 0; - if (dLen > 0 && CFH->write_func(data, dLen, CFH) != dLen) + if (dLen > 0 && CFH->write_func(data, dLen, CFH)) { /* if write didn't set errno, assume problem is no disk space */ if (errno == 0) @@ -382,7 +382,7 @@ _EndData(ArchiveHandle *AH, TocEntry *te) static void _PrintFileData(ArchiveHandle *AH, char *filename) { - size_t cnt; + size_t cnt = 0; char *buf; size_t buflen; CompressFileHandle *CFH; @@ -397,7 +397,7 @@ _PrintFileData(ArchiveHandle *AH, char *filename) buf = pg_malloc(ZLIB_OUT_SIZE); buflen = ZLIB_OUT_SIZE; - while ((cnt = CFH->read_func(buf, buflen, CFH))) + while (CFH->read_func(buf, buflen, &cnt, CFH) == 0 && cnt > 0) { ahwrite(buf, 1, cnt, AH); } @@ -491,7 +491,7 @@ _WriteByte(ArchiveHandle *AH, const int i) CompressFileHandle *CFH = ctx->dataFH; errno = 0; - if (CFH->write_func(&c, 1, CFH) != 1) + if (CFH->write_func(&c, 1, CFH)) { /* if write didn't set errno, assume problem is no disk space */ if (errno == 0) @@ -529,7 +529,7 @@ _WriteBuf(ArchiveHandle *AH, const void *buf, size_t len) CompressFileHandle *CFH = ctx->dataFH; errno = 0; - if (CFH->write_func(buf, len, CFH) != len) + if (CFH->write_func(buf, len, CFH)) { /* if write didn't set errno, assume problem is no disk space */ if (errno == 0) @@ -554,7 +554,7 @@ _ReadBuf(ArchiveHandle *AH, void *buf, size_t len) * If there was an I/O error, we already exited in readF(), so here we * exit on short reads. */ - if (CFH->read_func(buf, len, CFH) != len) + if (CFH->read_func(buf, len, NULL, CFH)) pg_fatal("could not read from input file: end of file"); } @@ -696,7 +696,7 @@ _EndLO(ArchiveHandle *AH, TocEntry *te, Oid oid) /* register the LO in blobs.toc */ len = snprintf(buf, sizeof(buf), "%u blob_%u.dat\n", oid, oid); - if (CFH->write_func(buf, len, CFH) != len) + if (CFH->write_func(buf, len, CFH)) pg_fatal("could not write to LOs TOC file"); } -- 2.34.1