Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap
Kunshchikov Vladimir wrote: > Hello Alvaro, > > here goes v4 version: removed unused header. > > Compilation of this code snippet with -Wall -Wexter -std=c89 doesn't produce > any warnings. Great, thanks! I have pushed this to all branches since 9.4. Would you please give it a look? Please let me know if you find any problems (particularly since you seem to have a test rig to verify it on corrupted files). I noticed that with this patch we no longer use WRITE_ERROR_EXIT in certain cases but instead do the printing/exiting directly. I think that's fine, but it would be neater to improve the WRITE_ERROR_EXIT macro so that it takes the cfp as an argument, and then the macro is in charge of calling get_cfp_error. But then I noticed that it wasn't very easy to improve things that way. I also noticed that the usage of mixed compressed/uncompressed file pointers in pg_dump is not very consistent, and it would take rather a lot of effort to clean up. So I gave up for now, particularly as a backpatchable bugfix. If you're interested in mop-up work, I think we can improve pg_dump some more there. At least, I think most common cases should correctly report any zlib problems now. Thanks for the patch! -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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
I haven't seen that but can't guarantee that such case does not exist 28 июля 2017 г. 9:19 PM пользователь "Alvaro Herrera" < alvhe...@2ndquadrant.com> написал: > Vladimir Kunschikov wrote: > > >This "maxlen" business and the fallback error message are > > >strange. We have roughly equivalent code in pg_basebackup.c > > >which has been working since 2011 > > >Perhaps you can drop the memchr/fallback tricks and adopt the > > >pg_basebackup coding? Or is there a specific reason to have > > >the memchr check? > > > > Ofcourse that tricks can be dropped, function will be much prettier. > > 'Tricks' were made to pass some strict internal tests. > > Well, if there are cases which cause zlib to return a message that > causes a crash when printing it or some other problem, then let's use > your version both in this new code and in pg_basebackup. Do these cases > really exist? > > > Initially I used exactly that function from pg_basebackup.c: > > https://github.com/kunschikov/postgres/commit/ > 15e9fda6df51cf17c0b0a4f201ee0f93cf258de9#diff- > 98e3f8ce5d6e87950dd66e4c8bdedb21R713 > > It was rewritten for the sake of somewhat exaggerated security. > > Version #5 in attachment. > > I'd rather have all the security we can get, so before dropping those > protections, let's make sure they are useless. > > -- > Álvaro Herrerahttps://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services >
Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap
Vladimir Kunschikov wrote: > >This "maxlen" business and the fallback error message are > >strange. We have roughly equivalent code in pg_basebackup.c > >which has been working since 2011 > >Perhaps you can drop the memchr/fallback tricks and adopt the > >pg_basebackup coding? Or is there a specific reason to have > >the memchr check? > > Ofcourse that tricks can be dropped, function will be much prettier. > 'Tricks' were made to pass some strict internal tests. Well, if there are cases which cause zlib to return a message that causes a crash when printing it or some other problem, then let's use your version both in this new code and in pg_basebackup. Do these cases really exist? > Initially I used exactly that function from pg_basebackup.c: > https://github.com/kunschikov/postgres/commit/15e9fda6df51cf17c0b0a4f201ee0f93cf258de9#diff-98e3f8ce5d6e87950dd66e4c8bdedb21R713 > It was rewritten for the sake of somewhat exaggerated security. > Version #5 in attachment. I'd rather have all the security we can get, so before dropping those protections, let's make sure they are useless. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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
>This "maxlen" business and the fallback error message are >strange. We have roughly equivalent code in pg_basebackup.c >which has been working since 2011 >Perhaps you can drop the memchr/fallback tricks and adopt the >pg_basebackup coding? Or is there a specific reason to have >the memchr check? Ofcourse that tricks can be dropped, function will be much prettier. 'Tricks' were made to pass some strict internal tests. Initially I used exactly that function from pg_basebackup.c: https://github.com/kunschikov/postgres/commit/15e9fda6df51cf17c0b0a4f201ee0f93cf258de9#diff-98e3f8ce5d6e87950dd66e4c8bdedb21R713 It was rewritten for the sake of somewhat exaggerated security. Version #5 in attachment. -- Regards, Vladimir Kunschikov diff --git a/src/bin/pg_dump/compress_io.c b/src/bin/pg_dump/compress_io.c index 991fe11..b8af1bc 100644 --- a/src/bin/pg_dump/compress_io.c +++ b/src/bin/pg_dump/compress_io.c @@ -709,5 +709,19 @@ 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; + const char *errmsg = gzerror(fp->compressedfp, &errnum); + if( errnum != Z_ERRNO) + return errmsg; + } #endif + return strerror(errno); +} diff --git a/src/bin/pg_dump/compress_io.h b/src/bin/pg_dump/compress_io.h index 8f2e752..06b3762 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 79922da..e9c26bc 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 f839712..b35d3fc 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
Kunshchikov Vladimir wrote: > Hello Alvaro, > > here goes v4 version: removed unused header. > > Compilation of this code snippet with -Wall -Wexter -std=c89 doesn't produce > any warnings. Great, thanks. +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); +} This "maxlen" business and the fallback error message are strange. We have roughly equivalent code in pg_basebackup.c, which has been working since 2011 (commit 048d148fe631), and it looks like this: #ifdef HAVE_LIBZ static const char * get_gz_error(gzFile gzf) { int errnum; const char *errmsg; errmsg = gzerror(gzf, &errnum); if (errnum == Z_ERRNO) return strerror(errno); else return errmsg; } #endif Perhaps you can drop the memchr/fallback tricks and adopt the pg_basebackup coding? Or is there a specific reason to have the memchr check? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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, 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
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 -- 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
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 -- 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
Kunshchikov Vladimir wrote: Hi, > 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. Yeah, I've complained about this before but never got around to actually fixing it. Thanks for the patch, I'll see about putting it on all branches soon. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers