close failures to stdout not detected

[Richard Brittain]

Using -c option, with stdout directed to a file on an AFS fileserver, write failures (typically due to quota-full errors) are not detected.
With AFS, some failures are not detected until close(), since writes go into a local cache. A quick perusal of the source suggests pbzip2 doesn't close() stdout.

[Richard Brittain (richard-brittain)]

This may affect NFS as well as AFS.
Suggested patch:

--- pbzip2.cpp 2014-04-10 03:01:54.000000001 +0000
+++ pbzip2.cpp-new 2014-04-10 02:50:09.000000001 +0000
@@ -4706,6 +4706,18 @@
    fprintf(stderr, "-------------------------------------------\n");
  } /* for */

+ // Explicit close on stdout if we've been writing there, after all input has been processed
+ if (OutputStdOut == 1)
+ {
+ ret = close(STDOUT_FILENO);
+ if (ret == -1)
+ {
+ ErrorContext::getInstance()->saveError();
+ handle_error(EF_EXIT, 1, "pbzip2: *ERROR: Failed to close output file! Aborting...\n");
+ exit(1);
+ }
+ }
+

[Yavor Nikolov (yavor-nikolov)]

@richard-brittain, thanks for reporting this!

I pushed a fix for that here: https://code.launchpad.net/~yavor-nikolov/pbzip2/bug-1300876-explicit-close-of-stdout
Would you be able to test it?
If all is working fine I think we'll be able to release version with that fix soon.

[Richard Brittain (richard-brittain)]

On Thu, 10 Apr 2014, Yavor Nikolov wrote:

> @richard-brittain, thanks for reporting this!

I love this program. It has made a huge difference to some of our work -
we generally use it for compressing multi-GB VM disk images.
I'm a bit rusty on coding - it has been a while since I looked at anything
as complex as this.

> I pushed a fix for that here: https://code.launchpad.net/~yavor-nikolov/pbzip2/bug-1300876-explicit-close-of-stdout
> Would you be able to test it?
> If all is working fine I think we'll be able to release version with that fix soon.

Thanks. I'll test this, but skimming your diffs, I think there may be too
many places where you removed the check for stdout vs named file.

Anywhere that we detect an error and are going to abort anyway, it would
be useful to close stout, but the exit fail status is probably already
going to get set. However, when looping over multiple input files which
complete without error, we don't want to close stdout inside the loop - in
this case it effectively concatenates the input files to a single
compressed output file (e.g., the code near line 2078 should retain the
'if (OutputStdOut == 0)' block.)

That's why I suggested an explicit close only for stdout, at the end of
the big 'for' loop, although I suspected there may be other places that
need it too.

We also need to still skip the writeFileMetaData() call on stdout, since
it isn't a real file.

Another very minor error - I think the error message for a close failure
is missing the word 'not'!

"pbzip2: *ERROR: Could close output file! Aborting..."

Richard

--
Richard Brittain, Research Computing Group,
                    IT Services, 37 Dewey Field Road, HB6219
                    Dartmouth College, Hanover NH 03755
<email address hidden> 6-2085

[Yavor Nikolov (yavor-nikolov)]

Thanks a lot for your feedback, Richard!

Yes, you're right - I added closing of stdout on too many places.
I'll give it a more careful look to see where exactly it needs to be closed. What matters is to ensure that we check exit code of the final close.

Yavor

[Yavor Nikolov (yavor-nikolov)]

I updated the code in my (bug-1300876-explicit-close-of-stdout) branch. I agree with our observation: for this particular issue it's best to close after the for loop.