close failures to stdout not detected

Bug #1300876 reported by Richard Brittain on 2014-04-01
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
pbzip2
Undecided
Yavor Nikolov

Bug Description

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.

Related branches

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);
+ }
+ }
+

Changed in pbzip2:
status: New → In Progress
assignee: nobody → Yavor Nikolov (yavor-nikolov)
Yavor Nikolov (yavor-nikolov) wrote :

@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.

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) wrote :

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) wrote :

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.

Changed in pbzip2:
status: In Progress → Fix Committed
milestone: none → 1.1.9
Changed in pbzip2:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers