Croak on failed write into a file

[ppisar]

From 7d8effc5b04adab4768615f8382e8f35dd8e110b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Petr=20P=C3=ADsa=C5=99?= <ppisar@redhat.com>
Date: Wed, 27 Nov 2013 10:45:39 +0100
Subject: [PATCH 1/2] Croak on failed write into a file
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The mirror() method saves a document into a file. Any error while
writing to the file, e.g. no disk space, was ignored. This patch fixes
it by croaking on such I/O error.

Signed-off-by: Petr Písař <ppisar@redhat.com>

---
 lib/HTTP/Tiny.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lib/HTTP/Tiny.pm b/lib/HTTP/Tiny.pm
index e81aa8f..5d494c0 100644
--- a/lib/HTTP/Tiny.pm
+++ b/lib/HTTP/Tiny.pm
@@ -209,7 +209,10 @@ sub mirror {
     open my $fh, ">", $tempfile
         or Carp::croak(qq/Error: Could not open temporary file $tempfile for downloading: $!\n/);
     binmode $fh;
-    $args->{data_callback} = sub { print {$fh} $_[0] };
+    $args->{data_callback} = sub {
+        print {$fh} $_[0]
+        or Carp::croak(qq/Error: Could not write into temporary file $tempfile: $!\n/);
+    };
     my $response = $self->request('GET', $url, $args);
     close $fh
         or Carp::croak(qq/Error: Could not close temporary file $tempfile: $!\n/);
-- 
1.8.3.1

[dagolden]

My understanding is that on most operating systems, printing errors won't be caught until close().

On what operating system did you observe a failure of the type you describe?

David

[ppisar]

On Wed, Nov 27, 2013 at 04:29:57AM -0800, David Golden wrote:

My understanding is that on most operating systems, printing errors won't be
caught until close().

The file handle is in binary mode, so there is no line buffering and perl will
call write(2) at regular intervals. Any of the write syscal can fail. E.g. if
your file system runs out disk of space.

On what operating system did you observe a failure of the type you describe?

I did not observe. This insufficiency was found by a security screenining the
code https://bugzilla.redhat.com/show_bug.cgi?id=1031096.

I can give a try to reproduce the failure, but I believe it's obvious it can
happen. Contrary I've never heard that major of operating system would delay
write(2) errors to the final close(2). At least POSIX.1-2001 shows large errno
list.

-- Petr

[dagolden]

The file handle is in binary mode, so there is no line buffering

You misunderstand what binary mode does. It removes any transformative PerlIO layers. It does not remove buffering.

[dagolden]

I've conferred with p5p and done some of my own testing. Errors during print() will be reported on close(). For example:

$ perl -wE 'open my $fh, ">", "/dev/full"; print $fh "foo"; close $fh or die "Error: $!"'
Error: No space left on device at -e line 1.

$ perl -wE 'open my $fh, ">", "/dev/full"; $fh->autoflush(1); print $fh "foo"; close $fh or die "Error: $!"'
Error: No space left on device at -e line 1.

$ perl -wE 'open my $fh, ">:unix", "/dev/full"; $fh->autoflush(1); print $fh "foo"; close $fh or die "Error: $!"'
Error: No space left on device at -e line 1.

The second example sets autoflush. The last sets autoflush and disables PerlIO buffering. All three report the print error at close.

[ppisar]

On Wed, Nov 27, 2013 at 03:26:07PM -0800, David Golden wrote:

I've conferred with p5p and done some of my own testing. Errors during print() will be reported on close(). For example:

$ perl -wE 'open my $fh, ">", "/dev/full"; print $fh "foo"; close $fh or die "Error: $!"'
Error: No space left on device at -e line 1.

$ perl -wE 'open my $fh, ">", "/dev/full"; $fh->autoflush(1); print $fh "foo"; close $fh or die "Error: $!"'
Error: No space left on device at -e line 1.

$ perl -wE 'open my $fh, ">:unix", "/dev/full"; $fh->autoflush(1); print $fh "foo"; close $fh or die "Error: $!"'
Error: No space left on device at -e line 1.

The second example sets autoflush. The last sets autoflush and disables
PerlIO buffering. All three report the print error at close.

IO::Handle lists clearerr() method, so there seems to be an error flag. Now
I did some tests with disk quota limiting disk usage and you seem to be right.
I always get error from close(). (Except when I try to write into a file
opened for reading only, but that's different case.)

Although not checking print() failure is safe, it wastenetwork bandwidth as
it will download a whole big file even a write error could been spotted much
sooner. I think aborting network transfer could be beneficial.

-- Petr