print doesn't overwrite $!

p5pRT commented 17 years ago

Migrated from rt.perl.org#43097 (status was 'open')

Searchable as RT43097$

p5pRT commented 17 years ago

From perlbug@der-pepe.de

Created by perlbug@der-pepe.de

This is a bug report for perl from perlbug@der-pepe.de\, generated with the help of perlbug 1.35 running under perl v5.8.8.

-----------------------------------------------------------------

open STDOUT\, '>/dev/full' or die $!; $|++; for (0..2) { print 'x' or warn $!; open my $f\, '\</no/path/to/file'; }

This script should print "No space left on device" three times (because this is what you get for writing to /dev/full.) However\, what it does print is this:

No space left on device at bug.pl line 4. No such file or directory at bug.pl line 4. No such file or directory at bug.pl line 4.

It seems that once "open" sets $! to "No such file or directory"\, "print" doesn't change $! any more even if it fails.

(The script only runs on Unix variants because of the /dev/full thing.)

Perl Info

``` Flags: category=core severity=low Site configuration information for perl v5.8.8: Configured by Gentoo at Sun May 13 23:05:56 CEST 2007. Summary of my perl5 (revision 5 version 8 subversion 8) configuration: Platform: osname=linux, osvers=2.6.20, archname=i686-linux uname='linux cyan 2.6.20 #3 preempt sat feb 10 22:34:23 cet 2007 i686 intel(r) pentium(r) m processor 2.00ghz genuineintel gnulinux ' config_args='-des -Darchname=i686-linux -Dcccdlflags=-fPIC -Dccdlflags=-rdynamic -Dcc=i686-pc-linux-gnu-gcc -Dprefix=/usr -Dvendorprefix=/usr -Dsiteprefix=/usr -Dlocincpth= -Doptimize=-O2 -fomit-frame-pointer -march=pentium-m -pipe -Duselargefiles -Dd_semctl_semun -Dscriptdir=/usr/bin -Dman1dir=/usr/share/man/man1 -Dman3dir=/usr/share/man/man3 -Dinstallman1dir=/usr/share/man/man1 -Dinstallman3dir=/usr/share/man/man3 -Dman1ext=1 -Dman3ext=3pm -Dinc_version_list=5.8.0 5.8.0/i686-linux 5.8.2 5.8.2/i686-linux 5.8.4 5.8.4/i686-linux 5.8.5 5.8.5/i686-linux 5.8.6 5.8.6/i686-linux 5.8.7 5.8.7/i686-linux -Dinc_version_list=5.8.0 5.8.0/i686-linux 5.8.2 5.8.2/i686-linux 5.8.4 5.8.4/i686-linux 5.8.5 5.8.5/i686-linux 5.8.6 5.8.6/i686-linux 5.8.7 5.8.7/i686-linux -Dcf_by=Gentoo -Ud_csh -Dusenm -Di_ndbm -Di_gdbm -Di_db' hint=recommended, useposix=true, d_sigaction=define usethreads=undef use5005threads=undef useithreads=undef usemultiplicity=undef useperlio=define d_sfio=undef uselargefiles=define usesocks=undef use64bitint=undef use64bitall=undef uselongdouble=undef usemymalloc=n, bincompat5005=undef Compiler: cc='i686-pc-linux-gnu-gcc', ccflags ='-fno-strict-aliasing -pipe -Wdeclaration-after-statement -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64', optimize='-O2 -fomit-frame-pointer -march=pentium-m -pipe', cppflags='-fno-strict-aliasing -pipe -Wdeclaration-after-statement' ccversion='', gccversion='4.1.1 (Gentoo 4.1.1-r1)', gccosandvers='' intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234 d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12 ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8 alignbytes=4, prototype=define Linker and Libraries: ld='i686-pc-linux-gnu-gcc', ldflags =' -L/usr/local/lib' libpth=/usr/local/lib /lib /usr/lib libs=-lpthread -lnsl -lndbm -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc perllibs=-lpthread -lnsl -ldl -lm -lcrypt -lutil -lc libc=/lib/libc-2.4.so, so=so, useshrplib=false, libperl=libperl.a gnulibc_version='2.4' Dynamic Linking: dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic' cccdlflags='-fPIC', lddlflags='-shared -L/usr/local/lib' Locally applied patches: @INC for perl v5.8.8: /etc/perl /usr/lib/perl5/vendor_perl/5.8.8/i686-linux /usr/lib/perl5/vendor_perl/5.8.8 /usr/lib/perl5/vendor_perl /usr/lib/perl5/site_perl/5.8.8/i686-linux /usr/lib/perl5/site_perl/5.8.8 /usr/lib/perl5/site_perl/5.8.7 /usr/lib/perl5/site_perl /usr/lib/perl5/5.8.8/i686-linux /usr/lib/perl5/5.8.8 /usr/local/lib/site_perl . Environment for perl v5.8.8: HOME=/home/pepe LANG (unset) LANGUAGE (unset) LC_ALL=en_US.ISO8859-1 LD_LIBRARY_PATH=/home/pepe/.lib:/home/pepe/root/lib LOGDIR (unset) PATH=/home/pepe/root/bin:/home/pepe/.bin:/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/i686-pc-linux-gnu/gcc-bin/4.1.1:/usr/i386-pc-linux-gnu/gcc-bin/3.4.6:/opt/ati/bin:/opt/blackdown-jdk-1.4.2.03/bin:/opt/blackdown-jdk-1.4.2.03/jre/bin:/usr/kde/3.5/bin:/usr/qt/3/bin:/usr/games/bin:/usr/local/winex/bin PERL_BADLANG (unset) SHELL=/bin/bash ```

p5pRT commented 17 years ago

From perlbug@der-pepe.de

I've tried strace-ing this\, and I found that buffering somehow gets re-enabled. These are the relevant system calls\, in order:

write(1\, "x"\, 1) = -1 ENOSPC (No space left on device) write(2\, "No space left on device at bug.p"...\, 42) = 42 open("/no/path/to/file"\, O_RDONLY|O_LARGEFILE) = -1 ENOENT (No such file or directory) open("/no/path/to/file"\, O_RDONLY|O_LARGEFILE) = -1 ENOENT (No such file or directory) open("/no/path/to/file"\, O_RDONLY|O_LARGEFILE) = -1 ENOENT (No such file or directory) write(1\, "xx"\, 2) = -1 ENOSPC (No space left on device)

Even if I include $|=1 in the loop just before "print"\, the "xx" is not printed seperatedly.

p5pRT commented 17 years ago

From @tux

On Thu\, 31 May 2007 18:39:08 -0700\, "perlbug@der-pepe.de (via RT)" \perlbug\-followup@perl\.org wrote:

# New Ticket Created by perlbug@der-pepe.de # Please include the string: [perl #43097] # in the subject line of all future correspondence about this issue. # \<URL: http://rt.perl.org/rt3/Ticket/Display.html?id=43097 >

This is a bug report for perl from perlbug@der-pepe.de\, generated with the help of perlbug 1.35 running under perl v5.8.8.

-----------------------------------------------------------------

open STDOUT\, '>/dev/full' or die $!; $|++; for (0..2) { print 'x' or warn $!; open my $f\, '\</no/path/to/file'; }

This script should print "No space left on device" three times (because this is what you get for writing to /dev/full.) However\, what it does print is this:

No space left on device at bug.pl line 4. No such file or directory at bug.pl line 4. No such file or directory at bug.pl line 4.

from perldoc perlvar: --8\<--- $! If used numerically\, yields the current value of the C "errno" variable\, or in other words\, if a system or library call fails\, it sets this variable. This means that the value of $! is meaningful only immediately after a failure: -->8---

the key here is *only immediately after the failure*. The last *system* failure in the second and third line was the fail open to the lexical handle. IMHO not a bug.

It seems that once "open" sets $! to "No such file or directory"\, "print" doesn't change $! any more even if it fails.

(The script only runs on Unix variants because of the /dev/full thing.)

-- H.Merijn Brand Amsterdam Perl Mongers (http://amsterdam.pm.org/) using & porting perl 5.6.2\, 5.8.x\, 5.9.x on HP-UX 10.20\, 11.00\, 11.11\, & 11.23\, SuSE 10.0 & 10.2\, AIX 4.3 & 5.2\, and Cygwin. http://qa.perl.org http://mirrors.develooper.com/hpux/ http://www.test-smoke.org http://www.goldmark.org/jeff/stupid-disclaimers/

p5pRT commented 17 years ago

The RT System itself - Status changed from 'new' to 'open'

p5pRT commented 17 years ago

From @demerphq

On 6/1/07\, H.Merijn Brand \h\.m\.brand@xs4all\.nl wrote:

On Thu\, 31 May 2007 18:39:08 -0700\, "perlbug@der-pepe.de (via RT)" \perlbug\-followup@perl\.org wrote:

# New Ticket Created by perlbug@der-pepe.de # Please include the string: [perl #43097] # in the subject line of all future correspondence about this issue. # \<URL: http://rt.perl.org/rt3/Ticket/Display.html?id=43097 >

This is a bug report for perl from perlbug@der-pepe.de\, generated with the help of perlbug 1.35 running under perl v5.8.8.

-----------------------------------------------------------------

open STDOUT\, '>/dev/full' or die $!; $|++; for (0..2) { print 'x' or warn $!; open my $f\, '\</no/path/to/file'; }

This script should print "No space left on device" three times (because this is what you get for writing to /dev/full.) However\, what it does print is this:

No space left on device at bug.pl line 4. No such file or directory at bug.pl line 4. No such file or directory at bug.pl line 4.

from perldoc perlvar: --8\<--- $! If used numerically\, yields the current value of the C "errno" variable\, or in other words\, if a system or library call fails\, it sets this variable. This means that the value of $! is meaningful only immediately after a failure: -->8---

the key here is *only immediately after the failure*. The last *system* failure in the second and third line was the fail open to the lexical handle. IMHO not a bug.

Since stdout is autoflushing each call to print should result in a system call which because of the magic of /dev/full should fail. Therefore it looks to me like it is a bug.

yves

-- perl -Mre=debug -e "/just|another|perl|hacker/"

p5pRT commented 16 years ago

From mail@der-pepe.de

open STDOUT\, '>/dev/full' or die $!; $|++; for (0..2) { print 'x' or warn $!; open my $f\, '\</no/path/to/file'; }

This script should print "No space left on device" three times (because this is what you get for writing to /dev/full.) However\, what it does print is this:

No space left on device at bug.pl line 4. No such file or directory at bug.pl line 4. No such file or directory at bug.pl line 4.

This bug is still in perl 5.8.8 and 5.10.0 and blead.

I think I have fixed this bug. After giving this careful thought\, I think that the patch below is the proper way to fix this; but I would appreciate feedback from someone with more experience.

The problem is:

pp_print calls do_print. If that is successful *and* the file-handle is autoflushing\, pp_print calls PerlIO_flush. The success of do_print depends on the handle's error flag\, which gets set when a flush fails and will never be cleared again when writing into the buffer; that means the filehandle will never be flushed again (until the buffer fills up). My fix simply clears the error flag before each write.

This has another implication: When you get an error from print and then print an empty string\, the second print will be successful. This hasn't been the case previously\, but I think it's the right behaviour.

I've also added tests for both the $! thing and the empty-string thing.

The tests don't use /dev/full as in the original report because I don't think that would be portable. Instead they produce a print error by printing to a closed pipe.

Regards\, Christoph

p5pRT commented 16 years ago

From mail@der-pepe.de

perlpatch

p5pRT commented 16 years ago

From @gisle

On Aug 18\, 2008\, at 18:40\, Christoph Bussenius via RT wrote:

open STDOUT\, '>/dev/full' or die $!; $|++; for (0..2) { print 'x' or warn $!; open my $f\, '\</no/path/to/file'; }

Is there any significance to the 'open my $f' statement here?

This script should print "No space left on device" three times (because this is what you get for writing to /dev/full.) However\,
what it does print is this:

No space left on device at bug.pl line 4. No such file or directory at bug.pl line 4. No such file or directory at bug.pl line 4.

This bug is still in perl 5.8.8 and 5.10.0 and blead.

I think I have fixed this bug. After giving this careful thought\, I think that the patch below is the proper way to fix this; but I would appreciate feedback from someone with more experience.

I don't think the patch is acceptable. If print fails the error
condition need to stay around until close() time. It's common to
just use print without testing its outcome and then in the end verify
that all prints succeeded by testing if close() is successful.

open(my $fh\, ">"\, $file) || die "Can't open $file: $!"; print $fh $buf; print $fh $buf; ... close($fh) || die "Can't write to $file: $!";

With your patch the first print could fail and then the second could
succeed; leading to close() not reporting the first error.

--Gisle

The problem is:

pp_print calls do_print. If that is successful *and* the file- handle is autoflushing\, pp_print calls PerlIO_flush. The success of do_print depends on the handle's error flag\, which gets set when a flush fails and will never be cleared again when writing
into the buffer; that means the filehandle will never be flushed again
(until the buffer fills up). My fix simply clears the error flag before each write.

This has another implication: When you get an error from print and
then print an empty string\, the second print will be successful. This
hasn't been the case previously\, but I think it's the right behaviour.

I've also added tests for both the $! thing and the empty-string
thing.

The tests don't use /dev/full as in the original report because I
don't think that would be portable. Instead they produce a print error by printing to a closed pipe.

Regards\, Christoph

\

p5pRT commented 16 years ago

From lists@der-pepe.de

On Tue\, Aug 19\, 2008 at 09:45:24AM +0200\, Gisle Aas wrote:

On Aug 18\, 2008\, at 18:40\, Christoph Bussenius via RT wrote:

open STDOUT\, '>/dev/full' or die $!; $|++; for (0..2) { print 'x' or warn $!; open my $f\, '\</no/path/to/file'; }

Is there any significance to the 'open my $f' statement here?

Yes\, it sets $! to ENOENT (No such file or directory)\, to demonstrate that print fails to reset $! to ENOSPC (No space left on device)\, as shown in the triple-quoted message:

This script should print "No space left on device" three times (because this is what you get for writing to /dev/full.) However\, what it does print is this:

No space left on device at bug.pl line 4. No such file or directory at bug.pl line 4. No such file or directory at bug.pl line 4.

(This was my original report for bug #43097).

I don't think the patch is acceptable. If print fails the error condition need to stay around until close() time. It's common to just use print without testing its outcome and then in the end verify that all prints succeeded by testing if close() is successful.
 open$my $fh\, ">"\, $file$ || die "Can't open $file&#8203;: $\!";
 print $fh $buf;
 print $fh $buf;
 \.\.\.
 close$$fh$ || die "Can't write to $file&#8203;: $\!";
With your patch the first print could fail and then the second could succeed; leading to close() not reporting the first error.

Hm\, I see your point. I'll think about it\, thanks.

Christoph

p5pRT commented 16 years ago

From lists@der-pepe.de

On Aug 18\, 2008\, at 18:40\, Christoph Bussenius via RT wrote:

pp_print calls do_print. If that is successful *and* the file-handle is autoflushing\, pp_print calls PerlIO_flush. The success of do_print depends on the handle's error flag\, which gets set when a flush fails and will never be cleared again when writing into the buffer; that means the filehandle will never be flushed again (until the buffer fills up). My fix simply clears the error flag before each write.

I've added a new patch below. Instead of clearing the error flag\, it will now avoid to consider the previous error flag for the return status of do_print. This fixes the bug and still keeps the error flag so that close knows about previous errors.

On Tue\, Aug 19\, 2008 at 09:45:24AM +0200\, Gisle Aas wrote:

It's common to just use print without testing its outcome and then in the end verify that all prints succeeded by testing if close() is successful.
 open$my $fh\, ">"\, $file$ || die "Can't open $file&#8203;: $\!";
 print $fh $buf;
 print $fh $buf;
 \.\.\.
 close$$fh$ || die "Can't write to $file&#8203;: $\!";
With your patch the first print could fail and then the second could succeed; leading to close() not reporting the first error.

This piece of code is still a bit problematic in my opinion. In close's error message\, $! is not guaranteed to describe the error that happened. If an error happens (as you suggest) in the first print\, $! will be something like "no space left on device". However\, in the passage you marked as "..."\, $! can end up with any strange error code\, and so the error that gets reported may not have anything to do with writing to the file.

I think the only way to get this to work properly is to save errno along with the error flag for every PerlIO handle.

Regards\, Christoph

Inline Patch

```diff --- perl-5.10.0/doio.c 2007-12-18 11:47:07.000000000 +0100 +++ perl-5.10.0-debug/doio.c 2008-09-08 12:32:57.325273275 +0200 @@ -1247,7 +1247,7 @@ if (len && (PerlIO_write(fp,tmps,len) == 0)) happy = FALSE; Safefree(tmpbuf); - return happy ? !PerlIO_error(fp) : FALSE; + return happy; } } --- perl-5.10.0/t/io/print.t 2007-12-18 11:47:08.000000000 +0100 +++ perl-5.10.0-debug/t/io/print.t 2008-08-18 17:49:05.093301791 +0200 @@ -6,10 +6,10 @@ } use strict 'vars'; -eval 'use Errno'; +eval 'use Errno; use IO::Handle'; die $@ if $@ and !$ENV{PERL_CORE_MINITEST}; -print "1..21\n"; +print "1..24\n"; my $foo = 'STDOUT'; print $foo "ok 1\n"; @@ -65,3 +65,31 @@ map print(+()), ('')x68; print "ok 21\n"; } + +# Create a closed pipe to test print's error reporting +# See bug #43097 +if (exists $IO::Handle::{autoflush}) { + if (pipe my $rpipe, my $wpipe) { + eval { $SIG{PIPE} = 'IGNORE'; }; + close $rpipe or die $!; + $wpipe->autoflush(1); + + # printing with autoflush to a closed pipe must fail + print $wpipe "xy" and print "not "; + print "ok 22\n"; + + open my $h, '< /nonexistent' and die '/nonexistent exists'; + + print $wpipe "xy"; + print 'not ' if $!{ENOENT}; + print "ok 23\n"; + + # Printing an empty string must not fail + print $wpipe '' or print 'not '; + print "ok 24\n"; + } else { + print "ok $_ # skipped: cannot pipe ($!)\n" for 22..24; + } +} else { + print "ok $_ # skipped: no autoflush\n" for 22..24; +} ```

p5pRT commented 11 years ago

From @tonycoz

On Mon Sep 08 04:07:40 2008\, lists@der-pepe.de wrote:

On Aug 18\, 2008\, at 18:40\, Christoph Bussenius via RT wrote:

pp_print calls do_print. If that is successful *and* the file-handle is autoflushing\, pp_print calls PerlIO_flush. The success of do_print depends on the handle's error flag\, which gets set when a flush fails and will never be cleared again when writing into the buffer; that means the filehandle will never be flushed again (until the buffer fills up). My fix simply clears the error flag before each write.

I've added a new patch below. Instead of clearing the error flag\, it will now avoid to consider the previous error flag for the return status of do_print. This fixes the bug and still keeps the error flag so that close knows about previous errors.

On Tue\, Aug 19\, 2008 at 09:45:24AM +0200\, Gisle Aas wrote:
It's common to just use print without testing its outcome and then in the end verify that all prints succeeded by testing if close() is successful.
 open$my $fh\, ">"\, $file$ || die "Can't open $file&#8203;: $\!";
 print $fh $buf;
 print $fh $buf;
 \.\.\.
 close$$fh$ || die "Can't write to $file&#8203;: $\!";
With your patch the first print could fail and then the second could succeed; leading to close() not reporting the first error.
This piece of code is still a bit problematic in my opinion. In close's error message\, $! is not guaranteed to describe the error that happened. If an error happens (as you suggest) in the first print\, $! will be something like "no space left on device". However\, in the passage you marked as "..."\, $! can end up with any strange error code\, and so the error that gets reported may not have anything to do with writing to the file.

I think the only way to get this to work properly is to save errno along with the error flag for every PerlIO handle.

Regards\, Christoph

--- perl-5.10.0/doio.c 2007-12-18 11:47:07.000000000 +0100 +++ perl-5.10.0-debug/doio.c 2008-09-08 12:32:57.325273275 +0200 @@ -1247\,7 +1247\,7 @@ if (len && (PerlIO_write(fp\,tmps\,len) == 0)) happy = FALSE; Safefree(tmpbuf); - return happy ? !PerlIO_error(fp) : FALSE; + return happy; } }

I believe this change is dangerous under the following circumstances:

- no autoflush to a full filesystem - program calls print() until the buffer fills\, at which point flush is called\, which fails\, setting the error flags. Note that the final write may only be partly copied to the buffer - file space is freed up - the next write forces a flush\, which writes the buffer with a possibly truncated final write to the file system - the new content is then written to the buffer (possibly flushing)\, and so on with new writes

This produces output with some content missing.

A patch that saves errno per file handle would be less dangerous\, but I don't know if we want perlio to go to that level to preserve error codes.

Tony

Perl / perl5