pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[patch] pg_dump/pg_restore zerror() and strerror() mishap
12+ messages / 3 participants
[nested] [flat]
* [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-06-22 08:46 Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
0 siblings, 2 replies; 12+ messages in thread
From: Kunshchikov Vladimir @ 2017-06-22 08:46 UTC (permalink / raw)
To: pgsql-hackers
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&idCom...;
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
[text/x-patch] strerror-gzerror-zlib.patch (4.7K, ../../1498120508308.9826@infotecs.ru/3-strerror-gzerror-zlib.patch)
download | inline diff:
commit 1578f5011ad22d78ae059a4ef0924426fd6db762
Author: Kunshchikov Vladimir <Kunshchikov.Vla@infotecs.ru>
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 <dirent.h>
#include <sys/stat.h>
+#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->compressedfp? get_gz_error(ctx->dataFH->compressedfp) : strerror(errno)); \
+ } while (0)
+
typedef struct
{
/*
diff --git a/src/bin/pg_dump/pg_backup_tar.c b/src/bin/pg_dump/pg_backup_tar.c
index a2b320f371..0b7f651537 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 <sys/stat.h>
#include <ctype.h>
@@ -556,7 +557,7 @@ _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))
exit_horribly(modulename,
- "could not read from input file: %s\n", strerror(errno));
+ "could not read from input file: %s\n", get_gz_error(th->zFH));
}
else
{
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-06-22 15:40 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
1 sibling, 0 replies; 12+ messages in thread
From: Alvaro Herrera @ 2017-06-22 15:40 UTC (permalink / raw)
To: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>; +Cc: pgsql-hackers
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 Herrera https://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
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-07-25 22:02 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
1 sibling, 1 reply; 12+ messages in thread
From: Alvaro Herrera @ 2017-07-25 22:02 UTC (permalink / raw)
To: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>; +Cc: pgsql-hackers
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 Herrera https://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
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-07-26 15:32 Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 12+ messages in thread
From: Kunshchikov Vladimir @ 2017-07-26 15:32 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers
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 <alvherre@2ndquadrant.com>
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 Herrera https://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
Attachments:
[text/x-patch] strerror-gzerror-zlib-v2.patch (3.1K, ../../1501082204231.82652@infotecs.ru/2-strerror-gzerror-zlib-v2.patch)
download | inline diff:
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 <sys/stat.h>
#include <ctype.h>
@@ -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
{
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-07-26 16:40 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
0 siblings, 1 reply; 12+ messages in thread
From: Alvaro Herrera @ 2017-07-26 16:40 UTC (permalink / raw)
To: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>; +Cc: pgsql-hackers
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 Herrera https://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
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-07-27 07:34 Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 1 reply; 12+ messages in thread
From: Kunshchikov Vladimir @ 2017-07-27 07:34 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers
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&idCom...;
________________________________
From: Alvaro Herrera <alvherre@2ndquadrant.com>
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 Herrera https://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
Attachments:
[text/x-patch] strerror-gzerror-zlib-v3.patch (3.1K, ../../26f687922d9f44ad98c2638e8a8a13de@infotecs.ru/3-strerror-gzerror-zlib-v3.patch)
download | inline diff:
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 <sys/stat.h>
#include <ctype.h>
@@ -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
{
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-07-28 08:29 Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
parent: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
0 siblings, 2 replies; 12+ messages in thread
From: Kunshchikov Vladimir @ 2017-07-28 08:29 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers
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&idCom...;
________________________________
From: Alvaro Herrera <alvherre@2ndquadrant.com>
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 Herrera https://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
Attachments:
[text/x-patch] strerror-gzerror-zlib-v4.patch (2.9K, ../../571149215d3c467dac8b000651507445@infotecs.ru/3-strerror-gzerror-zlib-v4.patch)
download | inline diff:
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
{
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-07-28 16:25 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
1 sibling, 0 replies; 12+ messages in thread
From: Alvaro Herrera @ 2017-07-28 16:25 UTC (permalink / raw)
To: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>; +Cc: pgsql-hackers
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 Herrera https://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
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-07-28 18:11 Vladimir Kunschikov <kunschikov@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Vladimir Kunschikov @ 2017-07-28 18:11 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; pgsql-hackers
>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-98e3f8ce...
It was rewritten for the sake of somewhat exaggerated security.
Version #5 in attachment.
--
Regards,
Vladimir Kunschikov
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachments:
[text/x-patch] strerror-gzerror-zlib-v5.patch (2.7K, ../../CAFWCTZa2o0xM9XFbKOPaWd=urONkCEmFimH9Dao474_q7P2ELg@mail.gmail.com/3-strerror-gzerror-zlib-v5.patch)
download | inline diff:
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
{
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-07-28 18:19 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Vladimir Kunschikov <kunschikov@gmail.com>
0 siblings, 1 reply; 12+ messages in thread
From: Alvaro Herrera @ 2017-07-28 18:19 UTC (permalink / raw)
To: Vladimir Kunschikov <kunschikov@gmail.com>; +Cc: pgsql-hackers
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-98e3f8ce...
> 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 Herrera https://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
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-07-28 18:41 Vladimir Kunschikov <kunschikov@gmail.com>
parent: Alvaro Herrera <alvherre@2ndquadrant.com>
0 siblings, 0 replies; 12+ messages in thread
From: Vladimir Kunschikov @ 2017-07-28 18:41 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@2ndquadrant.com>; +Cc: pgsql-hackers
I haven't seen that but can't guarantee that such case does not exist
28 июля 2017 г. 9:19 PM пользователь "Alvaro Herrera" <
alvherre@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 Herrera https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>
^ permalink raw reply [nested|flat] 12+ messages in thread
* Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap
@ 2017-08-02 22:36 Alvaro Herrera <alvherre@2ndquadrant.com>
parent: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
1 sibling, 0 replies; 12+ messages in thread
From: Alvaro Herrera @ 2017-08-02 22:36 UTC (permalink / raw)
To: Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>; +Cc: pgsql-hackers
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 Herrera https://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
^ permalink raw reply [nested|flat] 12+ messages in thread
end of thread, other threads:[~2017-08-02 22:36 UTC | newest]
Thread overview: 12+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2017-06-22 08:46 [patch] pg_dump/pg_restore zerror() and strerror() mishap Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
2017-06-22 15:40 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2017-07-25 22:02 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2017-07-26 15:32 ` Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
2017-07-26 16:40 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2017-07-27 07:34 ` Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
2017-07-28 08:29 ` Kunshchikov Vladimir <Vladimir.Kunshchikov@infotecs.ru>
2017-07-28 16:25 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2017-08-02 22:36 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2017-07-28 18:11 Re: [patch] pg_dump/pg_restore zerror() and strerror() mishap Vladimir Kunschikov <kunschikov@gmail.com>
2017-07-28 18:19 ` Alvaro Herrera <alvherre@2ndquadrant.com>
2017-07-28 18:41 ` Vladimir Kunschikov <kunschikov@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox