Implicit close()s are silently unchecked for error #9436
Comments
From tchrist@perl.comCreated by tchrist@chthon.perl.com ( Perhaps the severity of this bug should be "high", When Perl implicitly closes a filehandle, it neglects to detect and then Perl implicitly closes a filehandle under the following circumstances: (1) When you reopen a handle that's already open. The return (2) When an implicitly allocated but still open filehandle open(my style filehandles as well as those in the localized local *FH; (3) Prior to certain critical operations, such as fork(), flock(), (4) During global destruction, Perl chases down all the open handles END { close(STDOUT) || die "can't close STDOUT: $!" } Since this is all-but-compulsary on any program you want to It is not obvious the nature of the warning or error to report. Some However, the global destruction errors, *particularly* of STDOUT, are Below is the relevant portion of the originating discussion of this that Subject: Re: Creative and *routine* use of so-called "magic" ARGV (was [perl #2783] Security of ARGV using 2-argument open) Watch: % df -h . % ls -l it* % perl -i.orig -pe 'print "this is more stuff\n"' it % echo $? % ls -l it* To this day, Perl's implicit closing of files doesn't warn you of I've never convinced anybody this is important. Since *every* END { close(STDOUT) || die "can't close stdout: $!" } But that sort of thing should happen on all implicitly closed things. And Watch again, starting from before the bogus, ill-reported rename attempt: % df -h . % ls -l it % perl -e 'print "stuff\n"' >> it; echo $? % perl -e 'open(my % echo $? % ls -l it This is all incorrect behavior on Perl's part. Even cat knows better! % echo foo | cat >> it; echo $? +-------------------------------------------------------+ What's that about, eh? But I've been saying this for years, just like everything else. BTW, this proves my point about checking for print's status % perl -e 'open(my And you can't get the danged thing to detect its folly: % perl -WE 'open(my I firmly believe that this needs to be a part of *EVERY* Perl program, END { close(STDOUT) || die "can't close stdout: $!" } And I believe this *much* more than some here believe <ARGV> needs Perl Info |
From mmaslano@redhat.comCreated by mmaslano@redhat.comSummary: data are lost if there's not enough space on device. Original bug report: https://bugzilla.redhat.com/show_bug.cgi?id=576929 Original report from Jim Meyering follows: Version-Release number of selected component (if applicable): How reproducible: always (with a nearly-full partition) I've just done the following on a nearly-full partition: Start off with a two-byte file: $ echo b > j Use "perl -i" to filter that "in-place", increasing its size: $ perl -ni -e '/b/ and print "."x100000 . "\nlast\n"' j \ Perl didn't report any problem and has exited successfully. $ wc -c j $ tail -c 10 j; echo Confirmed. Truncated file, and write failure not reported. The above was done both with Fedora 12's v5.10.0 write(4, "................................"..., 4096) = 4096 Steps to Reproduce: as above Actual results: as above Expected results: perl reports the failure and does not destroy the input Perl Info |
From jim@meyering.net
I reported this in March, and it was forwarded to here: http://rt.perl.org/rt3/Public/Bug/Display.html?id=73830 I've just confirmed that 5.12.1 still suffers data loss when using -i on |
|
The RT System itself - Status changed from 'new' to 'open' |
From @kentfredricOut of curiosity, are there any cases you can think of where a Because I can't say I'd be incredibly fond of my program terminating Sure, for such cases you could alternatively prescribe the use of an un- It certainly seems to make sense to me that at least filehandles closed I can certainly expect that for some people, they might not care if |
|
The RT System itself - Status changed from 'new' to 'open' |
From alex.hartmaier@gmail.comOn Sat, Feb 16, 2013 at 8:24 PM, Kent Fredric via RT <
As Lukas Mai already wrote for read-only filehandles. What should go wrong |
From tchrist@perl.com
If the close does not reflect the error status on the handle, or if that
It's because Perl just mirrors errno. Yeah, it's annoying. That's why --tom |
From tchrist@perl.com"Kent Fredric via RT" <perlbug-followup@perl.org> wrote
No, not really.
Just as it is not possible to get reasonable behavior if you But that should not stop us from emitting a warning, I think. I always thought autodie should include this though. --tom |
From @cpansproutOn Fri Aug 01 11:02:42 2008, tom christiansen wrote:
Should it be just STDOUT, or should it also apply to ARGVOUT and the -- Father Chrysostomos |
From olga.kryzhanovska@gmail.comEINTR on close() can happen easily. The ksh93 developers learned that Olga On Sat, Feb 16, 2013 at 9:55 PM, Tom Christiansen <tchrist@perl.com> wrote:
-- |
From @cpansproutOn Fri Aug 01 11:02:42 2008, tom christiansen wrote:
I have implemented that part on the sprout/destroio branch, and also my $pid = open my $fh, "| perl -e0"; Output: I�m not so sure whether that should warn or not. And this is starting I would appreciate it if someone else could take over here, or at the Just running the test suite on that branch, flagging all the tests that Come to think of it, that requires no C knowledge. So lets add this to -- Father Chrysostomos |
From @cpansproutFrom 628daf8 Mon Sep 17 00:00:00 2001 If the implicit close() fails, warn about it, mentioning $! in the When compiling with -Accflags=-DPERL_NOISY_IO_DESTROY the warning This currently produces warnings for code like this: my $pid = open my $fh, "| perl -e0"; Output: I�m not so sure that should warn. I am not sufficiently versed in Inline Patch |
From @cpansproutOn Thu Jul 04 16:19:27 2013, olga.kryzhanovska@gmail.com wrote:
Thank you for the explanation. I hope someone will find it useful. -- Father Chrysostomos |
From @LeontOn Fri, Jul 5, 2013 at 1:18 AM, ол�га к��жанов�ка�
Fortunately, EINTR is easily handled by retrying.
SA_RESTART doesn't work well with our delayed signals. I wouldn't Leon |
From @cpansproutOn Fri Aug 01 11:02:42 2008, tom christiansen wrote:
I trying a stab at this once more, but I have run into a snag. I have created a very small disk image and I am trying to write a file to it. But notice how close() does not set $!: $ ./perl -Ilib -e 'open fh, ">/Volumes/Disk Image/foo"; print fh "x"x1000, "\n" for 1..50; close fh or die "error closing: $!"' $ ./perl -Ilib -e 'open fh, ">/Volumes/Disk Image/foo"; print fh "x"x1000, "\n" for 1..50; unlink "oenthueonth"; close fh or die "error closing: $!"' That last error has nothing to do with the false return value from close(). It is from an unrelated system call. So if I change perl to emit a default warning when an implicit close fails, then it cannot provide any real diagnostics as to why the handle failed. My attempts at adding it give this: $ ./perl -Ilib -e 'open fh, ">/Volumes/Disk Image/foo"; print fh "x"x1000, "\n" for 1..50; undef *fh' That�s fine, but without the explicit undef: $ ./perl -Ilib -e 'open fh, ">/Volumes/Disk Image/foo"; print fh "x"x1000, "\n" for 1..50; undef *fh' because at some point errno changed from 28 to 22 when I wasn�t looking. Omitting errno is very unhelpful: Error closing handle during global destruction. Would it be possible to modify handles to store the errno value instead of simply an error flag? Then close() could set it, and what people have been recommending for years (close or die $!) will start to do what people expect more reliably than before. -- Father Chrysostomos |
From tchrist@perl.com"Father Chrysostomos via RT" <perlbug-followup@perl.org> wrote
I hope so. Whether you justify it with DWIM or by calling not doing it a --tom |
From @cpansproutOn Mon Sep 15 12:35:42 2014, tom christiansen wrote:
I have implemented this on the sprout/destroio branch. I have tested it manually on Mac OS X, but the VMS and Windows code is completely untested. I see no way to automate a test for this. I would appreciate it if people with VMS and Windows could try this out, using a small disk image (or floppy disk if your computer still takes them), and see whether things work as expected. -- Father Chrysostomos |
From @cpansproutOn Thu Sep 18 00:08:39 2014, sprout wrote:
The Windows smokes from the smoke-me/destroio branch show that it won�t even compile there. I�ve not been able to figure out why. Any help would be appreciated. -- Father Chrysostomos |
From @cpansproutAs of dc0c4db, close() restores the value of $! relevant to the handle and implicitly closed handles emit a warning. The one remaining issue in this ticket is that STDOUT is not closed automatically on program exit. (I�m not sure I want to tackle it. This stuff is *hard*.) -- Father Chrysostomos |
From @jkeenanOn Sun Nov 02 20:14:13 2014, sprout wrote:
Father C: This ticket is marked as a blocker for 5.22.0. Given the age of the ticket, it's not a regression from 5.20. So should we still mark it as a blocker for 5.22? Thank you very much. -- |
From @steve-m-hayJust noting here (since I don't see the commit ID mentioned anywhere) that http://perl5.git.perl.org/perl.git/commitdiff/96d7c88819733eaaba892177a967d9e898b2b924 committed the major part of the work. However, the comment at https://rt-archive.perl.org/perl5/Ticket/Display.html?id=57512#txn-1316805 notes that one further piece of work is remaining, so the ticket should not be closed yet. Whether it's really a blocker for 5.22, however, is debatable, as James said, although tchrist made it clear in his original report that the problem in question -- not closing STDOUT on program exit -- is one of the most important cases to cover. |
From [Unknown Contact. See original ticket]Just noting here (since I don't see the commit ID mentioned anywhere) that http://perl5.git.perl.org/perl.git/commitdiff/96d7c88819733eaaba892177a967d9e898b2b924 committed the major part of the work. However, the comment at https://rt-archive.perl.org/perl5/Ticket/Display.html?id=57512#txn-1316805 notes that one further piece of work is remaining, so the ticket should not be closed yet. Whether it's really a blocker for 5.22, however, is debatable, as James said, although tchrist made it clear in his original report that the problem in question -- not closing STDOUT on program exit -- is one of the most important cases to cover. |
From @rjbsI've updated this ticket to block 5.24.0 instead. *plenty* of time! -- |
From @cpansproutOn Sun Mar 22 15:55:27 2015, rjbs wrote:
Not necessarily. I only worked on this bug because you said you wanted it fixed by 5.?? (don�t remember) because Tom Christiansen said convincingly that it was important. I am not comfortable doing anything really regarding I/O, and there was such a paucity of feedback, that I still do not know whether my implementation was correct. (You could say that people said they wanted this fixed but did not actually want it fixed, since they were not willing to put forth any effort, even as little as feedback and advice.) So, until some other victim steps forward, this ticket is effectively stalled. -- Father Chrysostomos |
From @rjbs* Father Chrysostomos via RT <perlbug-followup@perl.org> [2015-03-24T23:37:26]
You missed my \N{INVISIBLE WINKING FACE}. -- |
From @tonycozOn Tue Mar 24 20:37:25 2015, sprout wrote:
How about the attached? Testing the stdout flush issue would require looking for /dev/full, or something similar. No idea for a generic test for the in-place edit output failures, I used an almost full test disk. Tony |
From @tonycoz0001-perl-57512-report-an-error-and-fail-if-we-can-t-flus.patchFrom 7239299e6f7a645547d83946e6a9ff27701cefbb Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Wed, 4 Nov 2015 16:52:37 +1100
Subject: [perl #57512] report an error and fail if we can't flush stdout
---
perl.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/perl.c b/perl.c
index b64975b..9063645 100644
--- a/perl.c
+++ b/perl.c
@@ -584,6 +584,19 @@ perl_destruct(pTHXx)
assert(PL_scopestack_ix == 0);
/* Need to flush since END blocks can produce output */
+ /* flush stdout separately, since we can identify it */
+#ifdef USE_PERLIO
+ {
+ PerlIO *stdo = PerlIO_stdout();
+ if (*stdo && PerlIO_flush(stdo)) {
+ PerlIO_restore_errno(stdo);
+ PerlIO_printf(PerlIO_stderr(), "Unable to flush stdout: %s",
+ Strerror(errno));
+ if (!STATUS_UNIX)
+ STATUS_ALL_FAILURE;
+ }
+ }
+#endif
my_fflush_all();
#ifdef PERL_TRACE_OPS
--
2.1.4
|
From @tonycoz0002-perl-57512-croak-on-failure-to-close-an-in-place-edi.patchFrom 5435e20ac24d786e8d109dde8a073112ff8211ce Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 5 Nov 2015 15:06:00 +1100
Subject: [perl #57512] croak on failure to close an in-place edit output file
---
doio.c | 23 ++++++++++++++++++++++-
pod/perldiag.pod | 5 +++++
2 files changed, 27 insertions(+), 1 deletion(-)
diff --git a/doio.c b/doio.c
index d95ad9c..6704862 100644
--- a/doio.c
+++ b/doio.c
@@ -808,9 +808,13 @@ PerlIO *
Perl_nextargv(pTHX_ GV *gv, bool nomagicopen)
{
IO * const io = GvIOp(gv);
+ SV *const old_out_name = PL_inplace ? newSVsv(GvSV(gv)) : NULL;
PERL_ARGS_ASSERT_NEXTARGV;
+ if (old_out_name)
+ SAVEFREESV(old_out_name);
+
if (!PL_argvoutgv)
PL_argvoutgv = gv_fetchpvs("ARGVOUT", GV_ADD|GV_NOTQUAL, SVt_PVIO);
if (io && (IoFLAGS(io) & (IOf_ARGV|IOf_START)) == (IOf_ARGV|IOf_START)) {
@@ -852,6 +856,13 @@ Perl_nextargv(pTHX_ GV *gv, bool nomagicopen)
}
}
else {
+ {
+ IO * const io = GvIOp(PL_argvoutgv);
+ if (io && IoIFP(io) && old_out_name && !io_close(io, PL_argvoutgv, FALSE, FALSE)) {
+ Perl_croak(aTHX_ "Failed to close in-place edit file %"SVf": %s\n",
+ old_out_name, Strerror(errno));
+ }
+ }
/* This very long block ends with return IoIFP(GvIOp(gv));
Both this block and the block above fall through on open
failure to the warning code, and then the while loop above tries
@@ -1015,7 +1026,17 @@ Perl_nextargv(pTHX_ GV *gv, bool nomagicopen)
if (io && (IoFLAGS(io) & IOf_ARGV))
IoFLAGS(io) |= IOf_START;
if (PL_inplace) {
- (void)do_close(PL_argvoutgv,FALSE);
+ if (old_out_name) {
+ IO * const io = GvIOp(PL_argvoutgv);
+ if (io && IoIFP(io) && !io_close(io, PL_argvoutgv, FALSE, FALSE)) {
+ Perl_croak(aTHX_ "Failed to close in-place edit file %"SVf": %s\n",
+ old_out_name, Strerror(errno));
+ }
+ }
+ else {
+ /* maybe this is no longer wanted */
+ (void)do_close(PL_argvoutgv,FALSE);
+ }
if (io && (IoFLAGS(io) & IOf_ARGV)
&& PL_argvout_stack && AvFILLp(PL_argvout_stack) >= 0)
{
diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index 5111410..d7c6f8c 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -2167,6 +2167,11 @@ Check the #! line, or manually feed your script into Perl yourself.
CHECK, INIT, or END subroutine. Processing of the remainder of the
queue of such routines has been prematurely ended.
+=item Failed to close in-place edit file %s: %s
+
+(F) Closing an output file from in-place editing, as with the C<-i>
+command-line switch, failed.
+
=item False [] range "%s" in regex; marked by S<<-- HERE> in m/%s/
(W regexp)(F) A character class range must start and end at a literal
--
2.1.4
|
From @rjbsI have applied TonyC's patch tentatively. I have also moved this ticket to the v5.25.1 blockers for future consideration of final changes (like making it possible to fatalize just the implicit close failure warnings). -- |
From @khwilliamsonWell, it's almost time for 5.25.1, which this ticket blocks. The final message says it is so that final considerations can be done now. What might these be? -- |
From @rjbsOn Tue May 17 14:45:53 2016, khw wrote:
The only one that comes to mind *for me* is the one I referenced above. To elaborate, note these warnings from perldiag: Warning: unable to close filehandle properly: %s These warnings are in category "io". They cannot be fatalized without fatalizing quite a few other warnings. If I could "use warnings FATAL => 'io::implicitclose'" (or something), I'd use that instead of "close $f or die". Clearly this does not block 5.25.1 from being released, but it would be nice to have. -- |
From @cpansproutOn Tue May 17 15:06:24 2016, rjbs wrote:
I think I mentioned this before. You cannot really fatalize anything that happens automatically when exiting a scope. Any time that we die on scope exit, we end up with weird bugs, such as #105916. See also the double eval in ext/PerlIO-encoding/t/encoding.t and bug #115692. This type of problem comes up every so often, and there is no clear solution for it. Conceptually it doesn�t even make sense to me. If a handle closes because of a die() elsewhere, and the handle closure dies, too, which error wins? -- Father Chrysostomos |
From @iabynOn Tue, May 17, 2016 at 04:00:29PM -0700, Father Chrysostomos via RT wrote:
Note that your sample test case in that ticket passes under 5.24.0, -- |
From @cpansproutOn Wed May 18 02:09:55 2016, davem wrote:
Oh wow! Does that mean these sorts of things are fixable? I still don�t know which die() should win, though. -- Father Chrysostomos |
From @iabynOn Wed, May 18, 2016 at 06:25:42AM -0700, Father Chrysostomos via RT wrote:
One of my explicit goals for the context work was to make these sorts of All scope exits, whether normally via pp_leavefoo() or abnormally via cx = &cxstack[cxstack_ix]; Note that that whole chunk of code is idempotent, so that if an exception In particular, all the cx_popfoo()s' have been explicitly written to be cv = cx->blk_sub.cv; Currently the only difference between pp_leavefoo() and Perl_dounwind
It should behave the same way as a die in a STORE called during Which isn't actually answering your question ;-) -- |
From @iabynOn Mon, May 23, 2016 at 03:37:43PM +0100, Dave Mitchell wrote: Hi Karl! On 27 Jan you added this ticket to the 5.26.0 blockers. Was that -- |
From @khwilliamsonOn 03/20/2017 09:31 AM, Dave Mitchell wrote:
See https://rt-archive.perl.org/perl5/Ticket/Display.html?id=127731#txn-1443374 It was a 5.25 blocker; I moved it to instead block 5.26.0 so that it |
From @iabynOn Mon, Mar 20, 2017 at 10:28:42AM -0600, Karl Williamson wrote:
Ah thanks. In that case I propose that this ticket not be a blocker for 5.26.0, nor -- |
From @xsawyerxOn 03/20/2017 06:01 PM, Dave Mitchell wrote:
Agreed. |
Migrated from rt.perl.org#57512 (status was 'open')
Searchable as RT57512$
The text was updated successfully, but these errors were encountered: