Thread: pgsql: Minor pg_dump improvements
Minor pg_dump improvements Improve pg_dump by checking results on various fgetc() calls which previously were unchecked, ditto for ftello. Also clean up a couple of very minor memory leaks by waiting to allocate structures until after the initial check(s). Issues spotted by Coverity. Branch ------ master Details ------- http://git.postgresql.org/pg/commitdiff/cfa1b4a711dd03f824a9c3ab50911e61419d1eeb Modified Files -------------- src/bin/pg_dump/pg_backup_archiver.c | 27 +++++++++++++++++++++------ src/bin/pg_dump/pg_backup_custom.c | 5 ++++- src/bin/pg_dump/pg_dump.c | 8 ++++++-- 3 files changed, 31 insertions(+), 9 deletions(-)
Stephen Frost <sfrost@snowman.net> writes:
> Minor pg_dump improvements
I'm pretty sure you broke _CloseArchive with this hunk:
@@ -708,6 +708,9 @@ _CloseArchive(ArchiveHandle *AH)
{
WriteHead(AH);
tpos = ftello(AH->FH);
+ if (tpos < 0 || errno)
+ exit_horribly(modulename, "could not determine seek position in archive file: %s\n",
+ strerror(errno));
WriteToc(AH);
ctx->dataStart = _getFilePos(AH, ctx);
There's no reason to assume errno is zero at entry, and in any case
it should not be necessary to test for anything except tpos < 0.
I'm not sure why _ReopenArchive is coded like it was; seems like
tpos < 0 should be a sufficient test there too, and we shouldn't have to
reset errno beforehand.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Minor pg_dump improvements
>
> I'm pretty sure you broke _CloseArchive with this hunk:
That'd be pretty frustrating as my testing didn't exhibit any issues.
> @@ -708,6 +708,9 @@ _CloseArchive(ArchiveHandle *AH)
> {
> WriteHead(AH);
> tpos = ftello(AH->FH);
> + if (tpos < 0 || errno)
> + exit_horribly(modulename, "could not determine seek position in archive file: %s\n",
> + strerror(errno));
> WriteToc(AH);
> ctx->dataStart = _getFilePos(AH, ctx);
>
> There's no reason to assume errno is zero at entry, and in any case
> it should not be necessary to test for anything except tpos < 0.
Good point.
> I'm not sure why _ReopenArchive is coded like it was; seems like
> tpos < 0 should be a sufficient test there too, and we shouldn't have to
> reset errno beforehand.
Agreed, I'll update that also.
Thanks,
Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> I'm pretty sure you broke _CloseArchive with this hunk:
> That'd be pretty frustrating as my testing didn't exhibit any issues.
It's possible that errno is accidentally zero when we get there, but
the code as-committed surely can't be called bulletproof.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
> >> I'm pretty sure you broke _CloseArchive with this hunk:
>
> > That'd be pretty frustrating as my testing didn't exhibit any issues.
>
> It's possible that errno is accidentally zero when we get there, but
> the code as-committed surely can't be called bulletproof.
Agreed, oversight on my part.
The only remaining place we still clear errno in pg_dump is in
pg_backup_archive.c:checkSeek() around a similar ftello call, perhaps
that should be changed to check the result instead also?
Thanks,
Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes:
> The only remaining place we still clear errno in pg_dump is in
> pg_backup_archive.c:checkSeek() around a similar ftello call, perhaps
> that should be changed to check the result instead also?
Agreed; we should be using the same coding pattern wherever we call
ftello.
I suspect that this code may be left over from coping with some ancient
non-spec-compliant version of ftello? Probably not worth digging in
the archives to find out. The Single Unix Spec v2 says that the result
is (off_t) -1 on error, and we generally assume that platforms are at
least compliant with that.
grep shows me a couple of other places where the result of ftello doesn't
seem to be getting checked for error. Odd that Coverity didn't notice
those.
regards, tom lane
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > The only remaining place we still clear errno in pg_dump is in
> > pg_backup_archive.c:checkSeek() around a similar ftello call, perhaps
> > that should be changed to check the result instead also?
>
> Agreed; we should be using the same coding pattern wherever we call
> ftello.
Ok, I'll take care of that in a couple of hours (have to step out for a
bit).
> I suspect that this code may be left over from coping with some ancient
> non-spec-compliant version of ftello? Probably not worth digging in
> the archives to find out. The Single Unix Spec v2 says that the result
> is (off_t) -1 on error, and we generally assume that platforms are at
> least compliant with that.
I had been wondering the same, but agree w/ your assessment.
> grep shows me a couple of other places where the result of ftello doesn't
> seem to be getting checked for error. Odd that Coverity didn't notice
> those.
I'll try to figure out why it didn't, I would have expected it to. It's
possible I just hadn't gotten to those yet or someone decided they were
non-issues.
Thanks,
Stephen
Attachment
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> grep shows me a couple of other places where the result of ftello doesn't
> seem to be getting checked for error. Odd that Coverity didn't notice
> those.
At least two of those are in a #if 0 block... since 2001
(pg_backup_tar.c:_tarGetHeader). I'm thinking we may be better off
removing that code rather than continuing to pull it along (and update
it- there were at least three commits fixing things in that block
after it had been #if 0'd out). Another technically had a check, but
it was late, I've got a patch to improve that.
The last is inside a snprintf() which is just building a string to call
exit_horribly() with- seems like that's safe enough?
Thanks,
Stephen
Attachment
Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> grep shows me a couple of other places where the result of ftello doesn't
>> seem to be getting checked for error. Odd that Coverity didn't notice
>> those.
> At least two of those are in a #if 0 block... since 2001
> (pg_backup_tar.c:_tarGetHeader). I'm thinking we may be better off
> removing that code rather than continuing to pull it along
Works for me.
> The last is inside a snprintf() which is just building a string to call
> exit_horribly() with- seems like that's safe enough?
Agreed. Printing -1 seems like appropriate behavior there.
regards, tom lane
On 2014-02-09 02:29:25 +0000, Stephen Frost wrote:
> Minor pg_dump improvements
>
> Improve pg_dump by checking results on various fgetc() calls which
> previously were unchecked, ditto for ftello. Also clean up a couple
> of very minor memory leaks by waiting to allocate structures until
> after the initial check(s).
This triggers clang to emit:
/home/andres/src/postgresql/src/bin/pg_dump/pg_backup_archiver.c:1950:32: warning: comparison of constant -1 with
expressionof type
'ArchiveFormat' (aka 'enum _archiveFormat') is always false [-Wtautological-constant-out-of-range-compare]
if ((AH->format = fgetc(fh)) == EOF)
~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~
1 warning generated.
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--On 9. Februar 2014 13:55:03 -0500 Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Agreed; we should be using the same coding pattern wherever we call
> ftello.
>
> I suspect that this code may be left over from coping with some ancient
> non-spec-compliant version of ftello? Probably not worth digging in
> the archives to find out. The Single Unix Spec v2 says that the result
> is (off_t) -1 on error, and we generally assume that platforms are at
> least compliant with that.
>
> grep shows me a couple of other places where the result of ftello doesn't
> seem to be getting checked for error. Odd that Coverity didn't notice
> those.
It stroke me today that there's still something broken. pg_dump fails when
used in custom archive mode and piping to e.g. pg_restore:
pg_dump -Fc -p 5447 regression | pg_restore
pg_dump: [custom archiver] could not determine seek position in archive
file: Illegal seek
pg_restore: [custom archiver] unexpected end of file
pg_dump fails in _CloseArchive() with this hunk:
+ if (tpos < 0 || errno)
+ exit_horribly(modulename, "could not determine seek position in archive
file: %s\n",
+ strerror(errno));
errno is set to 29, which is ESPIPE and tpos was set to -1. If I read the
manpage on OSX here correctly, ftell[o] will always fail if used with a
pipe in this case:
[ESPIPE] The file descriptor underlying stream is associated
with a pipe or FIFO or file-position indicator
value is unspecified (see ungetc(3)).
A quick check shows that Debian has the same issue.
--
Thanks
Bernd
Bernd Helmle <mailings@oopsware.de> writes:
> It stroke me today that there's still something broken. pg_dump fails when
> used in custom archive mode and piping to e.g. pg_restore:
> pg_dump -Fc -p 5447 regression | pg_restore
> pg_dump: [custom archiver] could not determine seek position in archive
> file: Illegal seek
Ugh. Yeah, pg_dump to a pipe fails for me as well, and only in HEAD not
older versions. Will look.
regards, tom lane