Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap
Hello Alvaro, here goes v4 version: removed unused header. Compilation of this code snippet with -Wall -Wexter -std=c89 doesn't produce any warnings. -- Best regards, Vladimir Kunschikov Lead software developer IDS project InfoTeCS JSC From: Kunshchikov Vladimir Sent: Thursday, July 27, 2017 10:34:22 AM To: Alvaro Herrera Cc: pgsql-hackers@postgresql.org Subject: Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap Hello, Alvaro, thanks for the suggestions, attached version #3 with all of your requirements met. -- С уважением, Владимир Кунщиков Ведущий программист Отдел разработки систем обнаружения и предотвращения компьютерных атак Компания "ИнфоТеКС" <http://portal.infotecs.int/company/structure/Pages/detailsDepartment.aspx?idDepartment=235&idCompany=17> From: Alvaro Herrera Sent: Wednesday, July 26, 2017 7:40:20 PM To: Kunshchikov Vladimir Cc: pgsql-hackers@postgresql.org Subject: Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap Kunshchikov Vladimir wrote: > Hello Alvaro, thanks for the feedback, fixed all of your points. > Attached new version of patch. Looks great -- it's a lot smaller than the original even. One final touch -- see cfread(), where we have an #ifdef where we test for fp->compressedfp; the "#else" branch uses the same code as the !fp->compressedfp. I think your get_cfp_error can be written more simply using that form. (Also, your version probably errors or warns about "code before declaration" in that routine, which is not allowed in C89.) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c index 991fe11e8a..28e5b417c2 100644 --- a/src/bin/pg_dump/compress_io.c +++ b/src/bin/pg_dump/compress_io.c @@ -709,5 +709,22 @@ hasSuffix(const char *filename, const char *suffix) suffix, suffixlen) == 0; } +#endif +const char * +get_cfp_error(cfp* fp) +{ +#ifdef HAVE_LIBZ + if(fp->compressedfp){ + int errnum; + static const char fallback[] = "Zlib error"; + const int maxlen = 255; + const char *errmsg = gzerror(fp->compressedfp, &errnum); + if(!errmsg || !memchr(errmsg, 0, maxlen)) + errmsg = fallback; + + return errnum == Z_ERRNO ? strerror(errno) : errmsg; + } #endif + return strerror(errno); +} diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h index 8f2e752cba..06b3762233 100644 --- a/src/bin/pg_dump/compress_io.h +++ b/src/bin/pg_dump/compress_io.h @@ -65,5 +65,6 @@ extern int cfgetc(cfp *fp); extern char *cfgets(cfp *fp, char *buf, int len); extern int cfclose(cfp *fp); extern int cfeof(cfp *fp); +extern const char * get_cfp_error(cfp *fp); #endif diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c index 79922da8ba..e9c26bc571 100644 --- a/src/bin/pg_dump/pg_backup_directory.c +++ b/src/bin/pg_dump/pg_backup_directory.c @@ -352,7 +352,7 @@ _WriteData(ArchiveHandle *AH, const void *data, size_t dLen) lclContext *ctx = (lclContext *) AH->formatData; if (dLen > 0 && cfwrite(data, dLen, ctx->dataFH) != dLen) - WRITE_ERROR_EXIT; + exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH)); return; } @@ -490,7 +490,7 @@ _WriteByte(ArchiveHandle *AH, const int i) lclContext *ctx = (lclContext *) AH->formatData; if (cfwrite(&c, 1, ctx->dataFH) != 1) - WRITE_ERROR_EXIT; + exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH)); return 1; } @@ -519,7 +519,7 @@ _WriteBuf(ArchiveHandle *AH, const void *buf, size_t len) lclContext *ctx = (lclContext *) AH->formatData; if (cfwrite(buf, len, ctx->dataFH) != len) - WRITE_ERROR_EXIT; + exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH)); return; } diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c index f839712945..b35d3fcaa5 100644 --- a/src/bin/pg_dump/pg_backup_tar.c +++ b/src/bin/pg_dump/pg_backup_tar.c @@ -555,8 +555,12 @@ _tarReadRaw(ArchiveHandle *AH, void *buf, size_t len, TAR_MEMBER *th, FILE *fh) { res = GZREAD(&((char *) buf)[used], 1, len, th->zFH); if (res != len && !GZEOF(th->zFH)) +{ + int errnum; + const char *errmsg = gzerror(th->zFH, &errnum); exit_horribly(modulename, - "could not read from input file: %s\n", strerror(errno)); + "could not read from input file: %s\n", errnum == Z_ERRNO? strerror(errno) : errmsg); +} } else { -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap
Hello, Alvaro, thanks for the suggestions, attached version #3 with all of your requirements met. -- С уважением, Владимир Кунщиков Ведущий программист Отдел разработки систем обнаружения и предотвращения компьютерных атак Компания "ИнфоТеКС" <http://portal.infotecs.int/company/structure/Pages/detailsDepartment.aspx?idDepartment=235&idCompany=17> From: Alvaro Herrera Sent: Wednesday, July 26, 2017 7:40:20 PM To: Kunshchikov Vladimir Cc: pgsql-hackers@postgresql.org Subject: Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap Kunshchikov Vladimir wrote: > Hello Alvaro, thanks for the feedback, fixed all of your points. > Attached new version of patch. Looks great -- it's a lot smaller than the original even. One final touch -- see cfread(), where we have an #ifdef where we test for fp->compressedfp; the "#else" branch uses the same code as the !fp->compressedfp. I think your get_cfp_error can be written more simply using that form. (Also, your version probably errors or warns about "code before declaration" in that routine, which is not allowed in C89.) -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c index 991fe11e8a..28e5b417c2 100644 --- a/src/bin/pg_dump/compress_io.c +++ b/src/bin/pg_dump/compress_io.c @@ -709,5 +709,22 @@ hasSuffix(const char *filename, const char *suffix) suffix, suffixlen) == 0; } +#endif +const char * +get_cfp_error(cfp* fp) +{ +#ifdef HAVE_LIBZ + if(fp->compressedfp){ + int errnum; + static const char fallback[] = "Zlib error"; + const int maxlen = 255; + const char *errmsg = gzerror(fp->compressedfp, &errnum); + if(!errmsg || !memchr(errmsg, 0, maxlen)) + errmsg = fallback; + + return errnum == Z_ERRNO ? strerror(errno) : errmsg; + } #endif + return strerror(errno); +} diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h index 8f2e752cba..06b3762233 100644 --- a/src/bin/pg_dump/compress_io.h +++ b/src/bin/pg_dump/compress_io.h @@ -65,5 +65,6 @@ extern int cfgetc(cfp *fp); extern char *cfgets(cfp *fp, char *buf, int len); extern int cfclose(cfp *fp); extern int cfeof(cfp *fp); +extern const char * get_cfp_error(cfp *fp); #endif diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c index 79922da8ba..e9c26bc571 100644 --- a/src/bin/pg_dump/pg_backup_directory.c +++ b/src/bin/pg_dump/pg_backup_directory.c @@ -352,7 +352,7 @@ _WriteData(ArchiveHandle *AH, const void *data, size_t dLen) lclContext *ctx = (lclContext *) AH->formatData; if (dLen > 0 && cfwrite(data, dLen, ctx->dataFH) != dLen) - WRITE_ERROR_EXIT; + exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH)); return; } @@ -490,7 +490,7 @@ _WriteByte(ArchiveHandle *AH, const int i) lclContext *ctx = (lclContext *) AH->formatData; if (cfwrite(&c, 1, ctx->dataFH) != 1) - WRITE_ERROR_EXIT; + exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH)); return 1; } @@ -519,7 +519,7 @@ _WriteBuf(ArchiveHandle *AH, const void *buf, size_t len) lclContext *ctx = (lclContext *) AH->formatData; if (cfwrite(buf, len, ctx->dataFH) != len) - WRITE_ERROR_EXIT; + exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH)); return; } diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c index f839712945..f17ce78aca 100644 --- a/src/bin/pg_dump/pg_backup_tar.c +++ b/src/bin/pg_dump/pg_backup_tar.c @@ -35,6 +35,7 @@ #include "pgtar.h" #include "common/file_utils.h" #include "fe_utils/string_utils.h" +#include "compress_io.h" #include #include @@ -555,8 +556,12 @@ _tarReadRaw(ArchiveHandle *AH, void *buf, size_t len, TAR_MEMBER *th, FILE *fh) { res = GZREAD(&((char *) buf)[used], 1, len, th->zFH); if (res != len && !GZEOF(th->zFH)) +{ + int errnum; + const char *errmsg = gzerror(th->zFH, &errnum); exit_horribly(modulename, - "could not read from input file: %s\n", strerror(errno)); + "could not read from input file: %s\n", errnum == Z_ERRNO? strerror(errno) : errmsg); +} } else { -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap
Hello Alvaro, thanks for the feedback, fixed all of your points. Attached new version of patch. -- Best regards, Vladimir Kunschikov Lead software developer IDS project InfoTeCS JSC From: Alvaro Herrera Sent: Wednesday, July 26, 2017 1:02 AM To: Kunshchikov Vladimir Cc: pgsql-hackers@postgresql.org Subject: Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap Kunshchikov Vladimir wrote: > Errors should be like this: > pg_restore: [compress_io] could not read from input file: d3/2811.dat.gz: > invalid distance too far back > > Attached small fix for this issue. After looking at this patch, I don't like it very much. In particular, I don't like the way you've handled the WRITE_ERROR_EXIT macro in pg_backup_directory.c by undef'ing the existing one and creating it anew. The complete and correct definition should reside in one place (pg_backup_archiver.h), instead of being split in two and being defined differently. Another point is that you broke the comment on the definition of struct cfp "this is opaque to callers" by moving it to the header file precisely with the point of making it transparent to callers. We need some better idea there. I think it can be done by making compress_io.c responsible of handing over the error message through some new function (something very simple like "if compressedfp then return get_gz_error else strerror" should suffice). Also, I needed to rebase your patch over a recent pgindent run, though that's a pretty small change. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c index 991fe11e8a..67908a069c 100644 --- a/src/bin/pg_dump/compress_io.c +++ b/src/bin/pg_dump/compress_io.c @@ -709,5 +709,25 @@ hasSuffix(const char *filename, const char *suffix) suffix, suffixlen) == 0; } +#endif +const char * +get_cfp_error(cfp* fp) +{ +#ifdef HAVE_LIBZ + if(!fp->compressedfp) + return strerror(errno); + + int errnum; + + static const char fallback[] = "Zlib error"; + const int maxlen = 255; + const char *errmsg = gzerror(fp->compressedfp, &errnum); + if(!errmsg || !memchr(errmsg, 0, maxlen)) + errmsg = fallback; + + return errnum == Z_ERRNO ? strerror(errno) : errmsg; +#else + return strerror(errno); #endif +} diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h index 8f2e752cba..06b3762233 100644 --- a/src/bin/pg_dump/compress_io.h +++ b/src/bin/pg_dump/compress_io.h @@ -65,5 +65,6 @@ extern int cfgetc(cfp *fp); extern char *cfgets(cfp *fp, char *buf, int len); extern int cfclose(cfp *fp); extern int cfeof(cfp *fp); +extern const char * get_cfp_error(cfp *fp); #endif diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c index 79922da8ba..e9c26bc571 100644 --- a/src/bin/pg_dump/pg_backup_directory.c +++ b/src/bin/pg_dump/pg_backup_directory.c @@ -352,7 +352,7 @@ _WriteData(ArchiveHandle *AH, const void *data, size_t dLen) lclContext *ctx = (lclContext *) AH->formatData; if (dLen > 0 && cfwrite(data, dLen, ctx->dataFH) != dLen) - WRITE_ERROR_EXIT; + exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH)); return; } @@ -490,7 +490,7 @@ _WriteByte(ArchiveHandle *AH, const int i) lclContext *ctx = (lclContext *) AH->formatData; if (cfwrite(&c, 1, ctx->dataFH) != 1) - WRITE_ERROR_EXIT; + exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH)); return 1; } @@ -519,7 +519,7 @@ _WriteBuf(ArchiveHandle *AH, const void *buf, size_t len) lclContext *ctx = (lclContext *) AH->formatData; if (cfwrite(buf, len, ctx->dataFH) != len) - WRITE_ERROR_EXIT; + exit_horribly(modulename, "could not write to output file: %s\n", get_cfp_error(ctx->dataFH)); return; } diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c index f839712945..f17ce78aca 100644 --- a/src/bin/pg_dump/pg_backup_tar.c +++ b/src/bin/pg_dump/pg_backup_tar.c @@ -35,6 +35,7 @@ #include "pgtar.h" #include "common/file_utils.h" #include "fe_utils/string_utils.h" +#include "compress_io.h" #include #include @@ -555,8 +556,12 @@ _tarReadRaw(ArchiveHandle *AH, void *buf, size_t len, TAR_MEMBER *th, FILE *fh) { res = GZREAD(&((char *) buf)[used], 1, len, th->zFH); if (res != len && !GZEOF(th->zFH)) +{ + int errnum; + const char *errmsg = gzerror(th->zFH, &errnum); exit_horribly(modulename, - "could not read from input file: %s\n", strerror(errno)); + "could not read from input file: %s\n", errnum == Z_ERRNO? strerror(errno) : errmsg); +} } else { -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap
Hello, our testing team has noticed apparently wrong backup/restore error messages like this: pg_restore: [compress_io] could not read from input file: success pg_dump: [directory archiver] could not write to output file: success Such "success" messages are caused by calling strerror() after gzread()/gzwrite() failures. In order to properly decode errors, there should be used gzerror() instead of strerror(): http://refspecs.linuxbase.org/LSB_2.1.0/LSB-generic/LSB-generic/zlib-gzerror-1.html Errors should be like this: pg_restore: [compress_io] could not read from input file: d3/2811.dat.gz: invalid distance too far back Attached small fix for this issue. You can view that patch online on our github: https://github.com/Infotecs/postgres/commit/1578f5011ad22d78ae059a4ef0924426fd6db762 -- Best regards, Vladimir Kunschikov Lead software developer IDS project InfoTeCS JSC <http://portal.infotecs.int/company/structure/Pages/detailsDepartment.aspx?idDepartment=235&idCompany=17> commit 1578f5011ad22d78ae059a4ef0924426fd6db762 Author: Kunshchikov Vladimir Date: Wed Jun 21 11:33:47 2017 +0300 fixed gz_error/strerror mishap diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c index 631fa337e6..6e48d9d765 100644 --- a/src/bin/pg_dump/compress_io.c +++ b/src/bin/pg_dump/compress_io.c @@ -422,23 +422,6 @@ WriteDataToArchiveNone(ArchiveHandle *AH, CompressorState *cs, } -/*-- - * Compressed stream API - *-- - */ - -/* - * cfp represents an open stream, wrapping the underlying FILE or gzFile - * pointer. This is opaque to the callers. - */ -struct cfp -{ - FILE *uncompressedfp; -#ifdef HAVE_LIBZ - gzFile compressedfp; -#endif -}; - #ifdef HAVE_LIBZ static int hasSuffix(const char *filename, const char *suffix); #endif @@ -593,7 +576,7 @@ cfread(void *ptr, int size, cfp *fp) ret = gzread(fp->compressedfp, ptr, size); if (ret != size && !gzeof(fp->compressedfp)) exit_horribly(modulename, - "could not read from input file: %s\n", strerror(errno)); + "could not read from input file: %s\n", get_gz_error(fp->compressedfp)); } else #endif @@ -629,7 +612,7 @@ cfgetc(cfp *fp) { if (!gzeof(fp->compressedfp)) exit_horribly(modulename, - "could not read from input file: %s\n", strerror(errno)); + "could not read from input file: %s\n", get_gz_error(fp->compressedfp)); else exit_horribly(modulename, "could not read from input file: end of file\n"); @@ -710,4 +693,16 @@ hasSuffix(const char *filename, const char *suffix) suffixlen) == 0; } +const char * +get_gz_error(gzFile gzf) +{ + int errnum; + static const char fallback[] = "Zlib error"; + const int maxlen = 255; + const char *errmsg = gzerror(gzf, &errnum); + if(!errmsg || !memchr(errmsg, 0, maxlen)) + errmsg = fallback; + + return errnum == Z_ERRNO ? strerror(errno) : errmsg; +} #endif diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h index 8f2e752cba..91007b042b 100644 --- a/src/bin/pg_dump/compress_io.h +++ b/src/bin/pg_dump/compress_io.h @@ -27,6 +27,24 @@ typedef enum COMPR_ALG_LIBZ } CompressionAlgorithm; +/*-- + * Compressed stream API + *-- + */ + +/* + * cfp represents an open stream, wrapping the underlying FILE or gzFile + * pointer. This is opaque to the callers. + */ +typedef struct +{ + FILE *uncompressedfp; +#ifdef HAVE_LIBZ + gzFile compressedfp; +#endif +} cfp; + + /* Prototype for callback function to WriteDataToArchive() */ typedef void (*WriteFunc) (ArchiveHandle *AH, const char *buf, size_t len); @@ -52,10 +70,6 @@ extern void ReadDataFromArchive(ArchiveHandle *AH, int compression, extern void WriteDataToArchive(ArchiveHandle *AH, CompressorState *cs, const void *data, size_t dLen); extern void EndCompressor(ArchiveHandle *AH, CompressorState *cs); - - -typedef struct cfp cfp; - extern cfp *cfopen(const char *path, const char *mode, int compression); extern cfp *cfopen_read(const char *path, const char *mode); extern cfp *cfopen_write(const char *path, const char *mode, int compression); @@ -65,5 +79,6 @@ extern int cfgetc(cfp *fp); extern char *cfgets(cfp *fp, char *buf, int len); extern int cfclose(cfp *fp); extern int cfeof(cfp *fp); +extern const char * get_gz_error(gzFile gzf); #endif diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c index 79922da8ba..6196227a04 100644 --- a/src/bin/pg_dump/pg_backup_directory.c +++ b/src/bin/pg_dump/pg_backup_directory.c @@ -42,6 +42,15 @@ #include #include +#ifdef WRITE_ERROR_EXIT +#undef WRITE_ERROR_EXIT +#endif +#define WRITE_ERROR_EXIT \ + do { \ + exit_horribly(modulename, "could not write to output file: %s\n", \ + ctx && ctx->dataFH && ctx->dataFH->co