Обсуждение: [HACKERS] [patch] pg_dump/pg_restore zerror() and strerror() mishap
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.
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
Вложения
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
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
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 <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
Вложения
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 Herrera https://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/ 15e9fda6df51cf17c0b0a4f201ee0f 93cf258de9#diff- 98e3f8ce5d6e87950dd66e4c8bdedb 21R713
> 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