Re: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap

2017-08-02 Thread Alvaro Herrera
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

2017-07-28 Thread Vladimir Kunschikov
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

2017-07-28 Thread Alvaro Herrera
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

2017-07-28 Thread Vladimir Kunschikov
>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, );
 
+		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(, 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, );
 	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

2017-07-28 Thread Alvaro Herrera
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, );
+   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, );
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

2017-07-28 Thread Kunshchikov Vladimir
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=17>

From: Alvaro Herrera <alvhe...@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 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, );
+		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(, 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, );
 	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

2017-07-27 Thread Kunshchikov Vladimir

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=17>

From: Alvaro Herrera <alvhe...@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 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, );
+		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(, 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, );
 	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

2017-07-26 Thread Kunshchikov Vladimir
 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 <alvhe...@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 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, );
+	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(, 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, );
 	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

2017-07-26 Thread Alvaro Herrera
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

2017-07-25 Thread Alvaro Herrera
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

2017-06-22 Thread Alvaro Herrera
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


[HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap

2017-06-22 Thread Kunshchikov Vladimir
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


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, );
+	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->compressedfp? get_gz_error(ctx->dataFH->compressedfp) : strerror(errno)); \
+	} while (0)
+
 typedef struct
 {
 	/*
diff --git