Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

require should not clobber $! and $^E on success #13219

Open
p5pRT opened this issue Sep 2, 2013 · 11 comments
Open

require should not clobber $! and $^E on success #13219

p5pRT opened this issue Sep 2, 2013 · 11 comments
Labels
Closable? We might be able to close this ticket, but we need to check with the reporter distro-mswin32 type-core

Comments

@p5pRT
Copy link

p5pRT commented Sep 2, 2013

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

Searchable as RT119555$

@p5pRT
Copy link
Author

p5pRT commented Sep 2, 2013

From cm.perl@abtela.com

Created by cm.perl@abtela.com

On success, require resets $! and clobbers $^E (on platforms where $^E
is distinct from $!, such as Windows). It would be better to restore the
previous values of $! and $^E.

In the process of foraging through @​INC, require may try to open several
files, and when it succeeds on step N, $! and $^E carry the failure
codes for step N-1. Such failures are however part of the normal working
of require. There is no reason to report them to the user. This
consideration might have motivated the resetting of $! (although the
exact ratinale is unclear, see
http​://www.nntp.perl.org/group/perl.perl5.porters/2012/12/msg197015.html).
In any case $^E was left alone, and for the sake of consistency we might
consider resetting it also.

But resetting $! (and $^E) is a *bad thing* as it interferes with error
reporting. A number of core and CPAN modules use the following idiom (or
variants thereof) to implement 'lazy loading' of Carp :

sub croak {
  require Carp;
  goto &Carp​::croak;
}

The problem here is that on first invocation (assuming Carp has not been
loaded elsewhere), $! and $^E will have been corrupted to unusable
values, as the following demonstrates :

Taisha​:/cygdrive/d/perls/blead/perl-git/win32 $ cat
e​:/cm/perl/tests/badcarp/require_clobbering_errno.pl
use strict;
use warnings;

sub croak {
  require Carp;
  goto &Carp​::croak;
}

for (1 .. 3) {
  eval {
  rmdir 'notempty'
  or croak '$! = ', 0+$!, ', $^E = ', 0+$^E, "\n";
  1;
  } or do {
  my @​msg = split /\n/, $@​;
  print '$@​ : "', $msg[0], '"', "\n";
  print '$! : ', 0+$! , " ($!)\n";
  print '$^E : ', 0+$^E, " ($^E)\n";
  print "-----\n";
  }
}
Taisha​:/cygdrive/d/perls/blead/perl-git/win32 $ ../perl
e​:/cm/perl/tests/badcarp/require_clobbering_errno.pl
$@​ : "$! = 41, $^E = 145"
$! : 0 ()
$^E : 6 (Descripteur non valide)
-----
$@​ : "$! = 41, $^E = 145"
$! : 41 (Directory not empty)
$^E : 145 (Le répertoire n'est pas vide)
-----
$@​ : "$! = 41, $^E = 145"
$! : 41 (Directory not empty)
$^E : 145 (Le répertoire n'est pas vide)
-----
Taisha​:/cygdrive/d/perls/blead/perl-git/win32 $

Recent work on Carp (commit cbd58ba)
ensured that croak and confess hand out uncorrupted values of $! and
$^E, thus giving access to their numerical values for programmatic error
recovery (see
http​://www.nntp.perl.org/group/perl.perl5.porters/2013/08/msg206736.html
and
http​://www.nntp.perl.org/group/perl.perl5.porters/2012/12/msg197017.html
for a rationale).

In line with this effort, and considering the problem outlined above,
require should do the right thing and restore on success the values of
$! and $^E as they were on invocation.

ALTERNATIVE

If there is a compelling reason to keep having require corrupt^Hreset
$!, then $^E should probably also be reset for consistency (and both
resets should be documented). In addition, the idiom above should be
recast as

sub croak {
  { local ($!, $^E); require Carp }
  goto &Carp​::croak;
}

here is an indicative list of core files to patch :
dist/SelfLoader/lib/SelfLoader.pm
lib/POSIX.pm
lib/open.pm
lib/File/Compare.pm
lib/File/Copy.pm
lib/File/Path.pm
lib/attributes.pm
lib/_charnames.pm
lib/Tie/Memoize.pm
lib/SelfLoader.pm
lib/Term/Cap.pm
cpan/File-Path/lib/File/Path.pm
cpan/Term-Cap/Cap.pm
ext/attributes/attributes.pm
ext/POSIX/lib/POSIX.pm
ext/Tie-Memoize/lib/Tie/Memoize.pm

WORKAROUND

The problem above can be avoided by loading Carp as early as possible in
the main script (which I have been doing for years, after having been
bitten by this exact bug).

Perl Info

Flags:
     category=core
     severity=low

Site configuration information for perl 5.19.4:

Configured by cm at Mon Sep  2 01:34:49 2013.

Summary of my perl5 (revision 5 version 19 subversion 4) configuration:
   Commit id: cbd58baf5927dd469f38f80a7c76c8011150b6c5
   Platform:
     osname=MSWin32, osvers=4.0, archname=MSWin32-x64-multi-thread
     uname=''
     config_args='undef'
     hint=recommended, useposix=true, d_sigaction=undef
     useithreads=define, usemultiplicity=define
     useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
     use64bitint=define, use64bitall=undef, uselongdouble=undef
     usemymalloc=n, bincompat5005=undef
   Compiler:
     cc='gcc', ccflags =' -s -O2 -DWIN32 -DWIN64 -DCONSERVATIVE 
-DPERL_TEXTMODE_SCRIPTS -DPERL_IMPLICIT_CONTEXT -DPERL_IMPLICIT_SYS 
-DUSE_PERLIO -fno-strict-aliasing -mms-bitfields',
     optimize='-s -O2',
     cppflags='-DWIN32'
     ccversion='', gccversion='4.6.3', gccosandvers=''
     intsize=4, longsize=4, ptrsize=8, doublesize=8, byteorder=12345678
     d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
     ivtype='long long', ivsize=8, nvtype='double', nvsize=8, 
Off_t='long long', lseeksize=8
     alignbytes=8, prototype=define
   Linker and Libraries:
     ld='g++', ldflags ='-s -L"d:\perls\blead\perl\lib\CORE" 
-L"C:\MinGW\lib"'
     libpth=C:\MinGW\lib
     libs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool -lcomdlg32 
-ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid -lws2_32 -lmpr 
-lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
     perllibs=-lmoldname -lkernel32 -luser32 -lgdi32 -lwinspool 
-lcomdlg32 -ladvapi32 -lshell32 -lole32 -loleaut32 -lnetapi32 -luuid 
-lws2_32 -lmpr -lwinmm -lversion -lodbc32 -lodbccp32 -lcomctl32
     libc=, so=dll, useshrplib=true, libperl=libperl519.a
     gnulibc_version=''
   Dynamic Linking:
     dlsrc=dl_win32.xs, dlext=dll, d_dlsymun=undef, ccdlflags=' '
     cccdlflags=' ', lddlflags='-mdll -s 
-L"d:\perls\blead\perl\lib\CORE" -L"C:\MinGW\lib"'



@INC for perl 5.19.4:
     d:/perls/blead/perl/site/lib
     d:/perls/blead/perl/lib
     .


Environment for perl 5.19.4:
     CYGWIN=nodosfilewarning
     HOME=e:/cm
     LANG (unset)
     LANGUAGE (unset)
     LD_LIBRARY_PATH (unset)
     LOGDIR (unset)
 
PATH=C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0\;c:\strawberry-perl-5.16.1.1-64bit-portable\c\bin
     PERL_BADLANG (unset)
     SHELL (unset)

@p5pRT
Copy link
Author

p5pRT commented Sep 4, 2013

From cm.perl@abtela.com

Fist a big thanks to whoever took the pain to properly resend this
ticket to p5p (it apparently got lost). For the sake of those following
this on rt.perl.org, hereafter is a copy of a clarification exchange I
had with Zefram.

Le 02/09/2013 12​:55, Zefram a écrit :

Christian Millour wrote​:

In line with this effort, and considering the problem outlined above,
require should do the right thing and restore on success the values
of $! and $^E as they were on invocation.

This argument can be used to justify saving and restoring $! and $^E
around *any* operation. With Carp there was some historical
justification
that it had attempted to preserve $!, and I was willing to make that
work properly. require has never attempted to preserve $!, and in fact
the clearing suggests that it's actively attempting to signal its own
status through $!, which restoration would break.

-zefram

Damn. I have spent ages trying to try and craft this ticket so to avoid
this misunderstanding and didn't succeed . Sorry if I was unclear. I am
not advocating saving and restoring $! and $^E blindly within require,
but only when require is *successful*.

If the pseudocode in the pod is to be trusted, require signals errors by
*dying*, and obviously in that case $! and $^E sould *not* be restored,
so that the calling code may access those and react accordingly (see
http​://www.nntp.perl.org/group/perl.perl5.porters/2012/12/msg196988.html).

I am specifically *not* advocating using "local ($!, $^E);" at the start
of require, but rather the following pseudocode

sub require {
  my ($filename) = @​_;
  my @​saveerrs = ($!, $^E); # *added*
  if (exists $INC{$filename}) {
  return 1 if $INC{$filename};
  die "Compilation failed in require";
  }
  my ($realfilename,$result);
ITER​: {
  foreach $prefix (@​INC) {
  $realfilename = "$prefix/$filename";
  if (-f $realfilename) {
  $INC{$filename} = $realfilename;
  $result = do $realfilename;
  last ITER;
  }
  }
  die "Can't find $filename in \@​INC";
  }
  if ($@​) {
  $INC{$filename} = undef;
  die $@​;
  } elsif (!$result) {
  delete $INC{$filename};
  die "$filename did not return true value";
  } else {
  ($!, $^E) = @​saveerrs; # *added*
  return $result;
  }
}

which restores $! and $^E on success, and leaves them untouched for
inspection on failure.

Le 02/09/2013 13​:40, Zefram a écrit :

Christian Millour wrote​:

     I am not advocating saving and restoring $\! and $^E blindly

within require, but only when require is *successful*.

Thanks for the clarification. I did miss that detail of your original
message (which on rereading I can now see there). But it's still an
awfully broadly applicable argument. What else should work to preserve
$! on success? Or rather, what should not?

-zefram

for those on p5p who wonder what the hell we are talking about, and
awaiting sync between perlbug and p5p, the original report can be found at
https://rt-archive.perl.org/perl5/Ticket/Display.html?id=119555

To answer your question, my main focus with #116118 has been on trying
to ensure that croak/confess, as an error reporting mechanism, indeed
report faithfully on the error rather than corrupting it (in the sense
that $! and $^E are still fully available, especially their numerical
values, when the error is caught).

What I am really after is the following strategy for programmatic error
handling (see #116118 for the whole story) :

eval { some_sub_that_might_die_or_croak; 1 } or do {
  my ($evalerr, $e, $se) = ($@​, $!, $^E);
  if (ref $evalerr) {
  ... # deal with exception object
  } elsif (I_can_make_sense_of_this_error_string($evalerr)) {
  ... # deal with error string
  } else { # deal with numerical errors
  if ($e == ENOENT) {
  ...
  } elsif {$e == EACCES) {
  ...
  etc.
  }
};

[the underlying issue being that it is often impossible to make sense of
$evalerr above when you have to support different OS, versions of perl,
and locales, as the strings handed back by die/croak "oops​: $!, $^E" are
then essentially unpredictable]

Your "local ($!, $^E);" patch on longmess/shortmess went a long way
towards that goal. #119555 is intended as a complement, to prevent the
lurking bug currently inherent with the 'lazy loading of Carp' idiom.

One remaining problem is as follows​:
Le 31/12/2012 18​:27, Jan Dubois a écrit :

Even then you still have the problem that any DESTROY method run while
unwinding the stack back to the eval() handler may modify $! and $^E
again. So these values would also need to be handled similar to $@​.

Once this is solved I believe my concerns with error reporting will have
been handled, so the argument might be less broadly applicable as you think.

As a general rule, I think ops and subs and constructs should not
clobber (or reset) $! and $^E when successfull. But I also understand
that such a requirement might be overkill and too costly to be applied
bluntly. Still, I hope to raise some awareness of the utility of not
corrupting gratuitously $! and $^E.

In the specific case of require, I believe that fixing it as suggested
is easier and on the long term better than hunting for all instances of
the 'lazy loading of Carp' idiom. YMMV.

Christian

@p5pRT
Copy link
Author

p5pRT commented Jul 23, 2014

From cm.perl@abtela.com

Le 02/09/2013 11​:28, Christian Millour (via RT) a écrit :

-----------------------------------------------------------------
[Please describe your issue here]

On success, require resets $! and clobbers $^E (on platforms where $^E
is distinct from $!, such as Windows). It would be better to restore the
previous values of $! and $^E.

Let me restate this, and add some motivation. To recall, the problem
arises when you try to deal with the numerical values or $! and $^E for
programmatic error handling, rather than with their string counterpart.

Let us start with the baseline sample using die :

[cm@​COS63 ~/blead/perl-git]$ ./perl -E 'eval { $! = 21; die "fail​:$!"; 1 } or do { say "must handle numerical error ", 0+$!}'
must handle numerical error 21

So far so good.

We are taught about using Carp​::croak instead of die when authoring
modules. Thanks to some foresight from authors of Carp, and later work
by its maintainer (thanks, Zefram :-) ), Carp​::croak (and confess) do
attempt to preserve $!, so the error recovery strategy outlined above,
i.e. dealing with the numerical value of $!, can be used :

[cm@​COS63 ~/blead/perl-git]$ ./perl -E 'eval { use Carp; $! = 21; croak "fail​:$!"; 1 } or do { say "must handle numerical error ", 0+$!}'
must handle numerical error 21

what the croaking code cannot do however is 'autouse Carp', even though
this is the very example provided in the perldoc for autouse :

[cm@​COS63 ~/blead/perl-git]$ ./perl -E 'eval { use autouse Carp => qw(croak); $! = 21; croak "fail​:$!"; 1 } or do { say "must handle numerical error ", 0+$!}'
must handle numerical error 0

The problem arises through no fault of Carp or autouse, but because
require resets $! on success. This may be worked around by preloading Carp​:

[cm@​COS63 ~/blead/perl-git]$ ./perl -MCarp -E 'eval { use autouse Carp => qw(croak); $! = 21; croak "fail​:$!"; 1 } or do { say "must handle numerical error ", 0+$!}'
must handle numerical error 21

but that pretty much defeats the purpose of autouse, and makes for a
nasty trap for the unwary. And any pre-autouse code that uses a similar
mechanism, as in

package Whatever;
sub croak {
  require Carp;
  goto &Carp​::croak;
}

suffers from the same problem.

Possibly more serious, given e.g. how the latest edition of "Modern
Perl" by chromatic embraces autodie
(http​://www.modernperlbooks.com/mt/2012/10/why-the-modern-perl-book-uses-autodie.html),
the croaking code cannot 'use autodie' either​:

[cm@​COS63 ~/blead/perl-git]$ ./perl -MCarp -E 'eval { use autodie qw(open); open (A, ">", "."); 1 } or do { say "must handle numerical error ", 0+$!}'
must handle numerical error 0

again through no fault of autodie, but for the same reason as before
(require resetting $! on success). This may currently be worked around
by preloading autodie​::exception :

[cm@​COS63 ~/blead/perl-git]$ ./perl -Mautodie​::exception -E 'eval { use autodie qw(open); open (A, ">", "."); 1 } or do { say "must handle numerical error ", 0+$!}'
must handle numerical error 21

but is fragile and IMHO raises obscurity to a new level of darkness :-/

Actually, if the croaking code uses autodie, then the client code may
access the numerical value of $! from the autodie​::exception instance :

[cm@​COS63 ~/blead/perl-git]$ ./perl -E 'eval {use autodie qw(open); open (A, ">", "."); 1 } or do { say "must handle numerical error ", 0+$@​->errno}'
must handle numerical error 21

unfortunately the doc for errno in autodie​::exception states that 'This
method will leave the main autodie​::exception class' so it cannot be
relied on. Furthermore, autodie makes currently no provision for $^E.

On some systems $^E may be more specific than $!, and deserves to be
preserved as well. The examples above concentrated on $! for concision
but could obviously have been written with $^E instead.

----------- SO ? --------------

I can't think of a compelling reason for require to reset $! (and
corrupt $^E) when successful, instead of restoring their values on
entry. And indeed it seems to me that dSAVE_ERRNO, SAVE_ERRNO and
RESTORE_ERRNO exist in perl.h to deal with exactly such situations (and
furthermore DTRT wrt. $^E). An analysis together with a tentative patch
have been proposed here :
https://rt.perl.org/Public/Bug/Display.html?id=116118#txn-1181050 , and
no objection were raised.

Could this tentative patch please be updated as needed and
applied/smoked ? I can formalize a patch if needed but would rather have
the original author (Craig) get the credit.

Thanks for your time and consideration.

Christian

@p5pRT
Copy link
Author

p5pRT commented Jul 28, 2014

From @craigberry

On Wed, Jul 23, 2014 at 4​:00 AM, Christian Millour <cm.perl@​abtela.com> wrote​:

I can't think of a compelling reason for require to reset $! (and corrupt
$^E) when successful, instead of restoring their values on entry. And indeed
it seems to me that dSAVE_ERRNO, SAVE_ERRNO and RESTORE_ERRNO exist in
perl.h to deal with exactly such situations (and furthermore DTRT wrt. $^E).
An analysis together with a tentative patch have been proposed here :
https://rt.perl.org/Public/Bug/Display.html?id=116118#txn-1181050 , and no
objection were raised.

Could this tentative patch please be updated as needed and applied/smoked ?
I can formalize a patch if needed but would rather have the original author
(Craig) get the credit.

Thanks for your time and consideration.

The problem is my patch doesn't actually do what you want. It does
successfully restore any errno that is set in pp_require while rifling
through @​INC trying to find the file being required (assuming the file
is eventually found and successfully opened).

But after it's opened, the file is sent to S_doeval to be compiled,
and somewhere in there errno gets cleared again. I haven't figured
out exactly how or where yet. There is a call to CLEAR_ERRSV(), which
has been there in some form since Perl 5.000. That might be what's
clearing errno, or not; I don't know. It doesn't look like it would,
but maybe there's magic on PL_errgv or something.

On the surface, it sounds reasonable that a successful require should
leave errno in the state it was in prior to the require. But nothing
resembling a complete or well-though-out implementation has been
proposed, and any change in this area will involve touching sensitive
bits of the core that have been as they are for twenty years. A
reformulated patch is attached, but at best it's a partial solution.

For future reference, here's what you get with my patch​:

$ type foo.t
@​INC = qw(Perl Rules ../lib);

$e = $! = 13; $se = $^E = 13;
print '# $! is ' . (0+$!) . ' $^E is ' . (0+$^E) . "\n";
# Should look in 'Perl', then 'Rules' (unsuccessfully), then '../lib'
require 'version.pm';
print '# $! is ' . (0+$!) . ' $^E is ' . (0+$^E) . "\n";
$ perl foo.t
# $! is 13 $^E is 13
# $! is 0 $^E is 0

So you can see that errno is still cleared in require even though I
restore it after finding version.pm.

@p5pRT
Copy link
Author

p5pRT commented Jul 28, 2014

From @craigberry

0001-Restore-errno-after-INC-search-in-pp_require.patch
From 39094c4e6cf941efd0be9bc0d77eda99230dfd05 Mon Sep 17 00:00:00 2001
From: "Craig A. Berry" <craigberry@mac.com>
Date: Sat, 26 Jul 2014 12:44:42 -0500
Subject: [PATCH] Restore errno after @INC search in pp_require.

This addresses [perl #119555].  It's quite natural when looking
through @INC for a module that we don't always find it in the first
place we look and errno will have been set by a failed filesystem
lookup.  Historically we've always cleared errno when the lookup
is eventually successful so that we don't have a misleading value
hanging around.

This patch changes that behavior so we restore errno to what it was
before starting the @INC search rather than clearing it.
---
 pp_ctl.c              | 11 ++++++-----
 t/op/require_errors.t | 10 +++++++++-
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/pp_ctl.c b/pp_ctl.c
index 7d098b7..887e05d 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3676,8 +3676,9 @@ PP(pp_require)
     SV *hook_sv = NULL;
     SV *encoding;
     OP *op;
-    int saved_errno;
+    int doopen_errno;
     bool path_searchable;
+    dSAVE_ERRNO;
 
     sv = POPs;
     if ( (SvNIOKp(sv) || SvVOK(sv)) && PL_op->op_type != OP_DOFILE) {
@@ -4022,13 +4023,13 @@ PP(pp_require)
 	    }
 	}
     }
-    saved_errno = errno; /* sv_2mortal can realloc things */
+    doopen_errno = errno; /* sv_2mortal can realloc things */
     sv_2mortal(namesv);
     if (!tryrsfp) {
 	if (PL_op->op_type == OP_REQUIRE) {
-	    if(saved_errno == EMFILE || saved_errno == EACCES) {
+	    if(doopen_errno == EMFILE || doopen_errno == EACCES) {
 		/* diag_listed_as: Can't locate %s */
-		DIE(aTHX_ "Can't locate %s:   %s", name, Strerror(saved_errno));
+		DIE(aTHX_ "Can't locate %s:   %s", name, Strerror(doopen_errno));
 	    } else {
 	        if (namesv) {			/* did we lookup @INC? */
 		    AV * const ar = GvAVn(PL_incgv);
@@ -4072,7 +4073,7 @@ PP(pp_require)
 	RETPUSHUNDEF;
     }
     else
-	SETERRNO(0, SS_NORMAL);
+	RESTORE_ERRNO;
 
     /* Assume success here to prevent recursive requirement. */
     /* name is never assigned to again, so len is still strlen(name)  */
diff --git a/t/op/require_errors.t b/t/op/require_errors.t
index a152d2d..09a7d67 100644
--- a/t/op/require_errors.t
+++ b/t/op/require_errors.t
@@ -7,7 +7,7 @@ BEGIN {
     require './test.pl';
 }
 
-plan(tests => 17);
+plan(tests => 19);
 
 my $nonfile = tempfile();
 
@@ -133,3 +133,11 @@ like $@, qr/^Can't locate strict\.pm\\0invalid: /, 'do nul check';
 eval "require strict\0::invalid;";
 like $@, qr/^syntax error at \(eval \d+\) line 1/, 'parse error with \0 in barewords module names';
 
+{
+  local ($!, $^E) = my ($e, $se) = (1, 1);
+  # Should look in 'Perl', then 'Rules' (unsuccessfully), then '../lib'
+  require 'version.pm';
+  is (0+$! , $e , q{require does not obviously clobber $!});
+  is (0+$^E, $se, q{require does not obviously clobber $^E})
+         or diag ("BTW, " .  (0+$^E) . " is '$^E'");
+}
-- 
1.8.4.2

@p5pRT
Copy link
Author

p5pRT commented Jul 28, 2014

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

@p5pRT
Copy link
Author

p5pRT commented Jul 28, 2014

From cm.perl@abtela.com

Le 28/07/2014 05​:01, Craig A. Berry a écrit :

The problem is my patch doesn't actually do what you want. It does
successfully restore any errno that is set in pp_require while rifling
through @​INC trying to find the file being required (assuming the file
is eventually found and successfully opened).

But after it's opened, the file is sent to S_doeval to be compiled,
and somewhere in there errno gets cleared again. I haven't figured
out exactly how or where yet. There is a call to CLEAR_ERRSV(), which
has been there in some form since Perl 5.000. That might be what's
clearing errno, or not; I don't know. It doesn't look like it would,
but maybe there's magic on PL_errgv or something.

On the surface, it sounds reasonable that a successful require should
leave errno in the state it was in prior to the require. But nothing
resembling a complete or well-though-out implementation has been
proposed, and any change in this area will involve touching sensitive
bits of the core that have been as they are for twenty years. A
reformulated patch is attached, but at best it's a partial solution.

Thank you for your time and efforts on this.

pp_require is indeed quite involved and I fully understand anyone being
wary of touching it.

Still, according to the documentation, require dies on error, and
otherwise survives. So irrespective of the internal causes of changes to
$! and $^E, it should be OK to restore their values on entry in those
cases where require does not die. In Perl :

sub new_require {
  my ($e, $se) = ($!, $^E);
  old_require $_[0]; # dies on error...
  ($!, $^E) = ($e, $se); # ... and otherwise restore $! and $^E
}

While not strictly equivalent, and as an experiment, I wrapped the
existing pp_require, renamed pp_require1 but otherwise untouched, into a
new pp_require tasked with saving/restoring $! and $^E :

PP(pp_require1)
{
... existing code
}
PP(pp_require)
{
  OP *op;
  dSAVE_ERRNO;
  op = Perl_pp_require1(
#ifndef PERL_IS_MINIPERL
  my_perl
#endif
  );
  if ((*_errno()) == 0)
  RESTORE_ERRNO;
  return op;
}

However crude and ill-mannered, this hack seemed to work. A variation on
the above might be the best solution. A similar idea, implemented in the
attached patch, is to dSAVE_ERRNO as you did, but to RESTORE_ERRNO
before any return point that is not a DIE. I find four of those : two
RETPUSHYES, one RETPUSHUNDEF, and the final return. I can't see how
those RESTORE_ERRNOs could damage the working of pp_require, even with
the recursive calls that seem to occur (via S_doeval). Note that the
SETERRNO(0, SS_NORMAL) introduced in
<http​://perl5.git.perl.org/perl.git/commit/d8bfb8bddf933a815b590823bd52295534e6ded0?f=pp_ctl.c>
has been removed because 1) it seems unrelated to the internal mechanism
of pp_require, and 2) its effect would be nullified by the
RESTORE_ERRNOs (at least on success).

This seems to work (one.pm is successfully required, without clobbering
$! and $^E) :

blead(w32) $ echo 0 > zero.pm; echo 1 > one.pm
blead(w32) $ ../perl -IPerl -IRule -e 'for (@​ARGV) { $! = $^E = 1; eval
{require "$_"; 1} or print $@​; print $_,q{ ==&gt; $! = }, 0+$!, q{, $^E =
}, 0+$^E, qq{ [$^E]\n}}' doesnotexist.pm zero.pm one.pm
Can't locate doesnotexist.pm in @​INC (you may need to install the
doesnotexist module) (@​INC contains​: Perl Rule
D​:/perls/blead/perl-git2/lib .) at -e line 1.
doesnotexist.pm ==> $! = 2, $^E = 2 [Le fichier sp cifi est introuvable]
zero.pm did not return a true value at -e line 1.
zero.pm ==> $! = 1, $^E = 1 [Fonction incorrecte]
one.pm ==> $! = 1, $^E = 1 [Fonction incorrecte]
blead(w32) $

This is better than the stock perl (here strawberry perl portable 5.20)

Perl_5.20.0 $ echo 0 > zero.pm; echo 1 > one.pm
Perl_5.20.0 $ perl -IPerl -IRule -e 'for (@​ARGV) { $! = $^E = 1; eval
{require "$_"; 1} or print $@​; print $_,q{ ==&gt; $! = }, 0+$!, q{, $^E =
}, 0+$^E, qq{ [$^E]\n}}' doesnotexist.pm zero.pm one.pm
Can't locate doesnotexist.pm in @​INC (you may need to install the
doesnotexist module) (@​INC contains​: Perl Rule
C​:/strawberry-perl-5.20.0.1-32bit-portable-PDL/perl/site/lib/MSWin32-x86-multi-thread-64int
C​:/strawberry-perl-5.20.0.1-32bit-portable-PDL/perl/site/lib
C​:/strawberry-perl-5.20.0.1-32bit-portable-PDL/perl/vendor/lib
C​:/strawberry-perl-5.20.0.1-32bit-portable-PDL/perl/lib .) at -e line 1.
doesnotexist.pm ==> $! = 2, $^E = 2 [Le fichier spécifié est introuvable]
zero.pm did not return a true value at -e line 1.
zero.pm ==> $! = 0, $^E = 6 [Descripteur non valide]
one.pm ==> $! = 0, $^E = 6 [Descripteur non valide]
Perl_5.20.0 $

In case of failure the values of $! and $^E do not seem much worse with
the patch than without (given that for "did not return a true value"
they are not set anyway, so may be a bit random...).

Comments/critics/smokes warmly welcome :-) Best regards,

Christian

@p5pRT
Copy link
Author

p5pRT commented Jul 28, 2014

From cm.perl@abtela.com

0001-Restore-errno-on-succesfull-require-199555.patch
From e6eea131fb645accf4e2d8cea693ad1c7709fbe7 Mon Sep 17 00:00:00 2001
From: Christian Millour <cm.perl@abtela.com>
Date: Mon, 28 Jul 2014 23:34:22 +0200
Subject: [PATCH] Restore errno on succesfull require (#199555)

Historically, require used to clear errno ($!) on success. While errno
(and on some platforms $^E) might be set several times during a
require (such as when searching through @INC and failing on the first
directories), such temporary errors should not be visible to the user
when require finally succeeds. Clearing errno is wrong however, as
this produces pernicious action-at-a-distance effects (see #199555).
Not doing anything, as was the case with $^E, was no better as it left
$^E in a semi-random state. This patch addresses these issues by
saving $! and $^E on entry to require, and restoring their values on
successfull exit.
---
 pp_ctl.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/pp_ctl.c b/pp_ctl.c
index 7d098b7..9975be2 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3676,8 +3676,9 @@ PP(pp_require)
     SV *hook_sv = NULL;
     SV *encoding;
     OP *op;
-    int saved_errno;
+    int doopen_errno;
     bool path_searchable;
+    dSAVE_ERRNO;
 
     sv = POPs;
     if ( (SvNIOKp(sv) || SvVOK(sv)) && PL_op->op_type != OP_DOFILE) {
@@ -3735,6 +3736,7 @@ PP(pp_require)
 	    }
 	}
 
+	RESTORE_ERRNO;
 	RETPUSHYES;
     }
     name = SvPV_const(sv, len);
@@ -3777,9 +3779,10 @@ PP(pp_require)
 	SV * const * const svp = hv_fetch(GvHVn(PL_incgv),
 					  unixname, unixlen, 0);
 	if ( svp ) {
-	    if (*svp != &PL_sv_undef)
+	    if (*svp != &PL_sv_undef) {
+		RESTORE_ERRNO;
 		RETPUSHYES;
-	    else
+            } else
 		DIE(aTHX_ "Attempt to reload %s aborted.\n"
 			    "Compilation failed in require", unixname);
 	}
@@ -4022,13 +4025,13 @@ PP(pp_require)
 	    }
 	}
     }
-    saved_errno = errno; /* sv_2mortal can realloc things */
+    doopen_errno = errno; /* sv_2mortal can realloc things */
     sv_2mortal(namesv);
     if (!tryrsfp) {
 	if (PL_op->op_type == OP_REQUIRE) {
-	    if(saved_errno == EMFILE || saved_errno == EACCES) {
+	    if(doopen_errno == EMFILE || doopen_errno == EACCES) {
 		/* diag_listed_as: Can't locate %s */
-		DIE(aTHX_ "Can't locate %s:   %s", name, Strerror(saved_errno));
+		DIE(aTHX_ "Can't locate %s:   %s", name, Strerror(doopen_errno));
 	    } else {
 	        if (namesv) {			/* did we lookup @INC? */
 		    AV * const ar = GvAVn(PL_incgv);
@@ -4069,10 +4072,9 @@ PP(pp_require)
 	}
 
 	CLEAR_ERRSV();
+	RESTORE_ERRNO;
 	RETPUSHUNDEF;
     }
-    else
-	SETERRNO(0, SS_NORMAL);
 
     /* Assume success here to prevent recursive requirement. */
     /* name is never assigned to again, so len is still strlen(name)  */
@@ -4131,6 +4133,7 @@ PP(pp_require)
 
     LOADED_FILE_PROBE(unixname);
 
+    RESTORE_ERRNO;
     return op;
 }
 
-- 
1.7.4

@p5pRT
Copy link
Author

p5pRT commented Jul 28, 2014

From cm.perl@abtela.com

minor edit on the patch file to reference the proper RT ticket (119...
instead of 199...). Sorry for the noise.

Christian

Le 29/07/2014 00​:46, Christian Millour a écrit :

Le 28/07/2014 05​:01, Craig A. Berry a écrit :

The problem is my patch doesn't actually do what you want. It does
successfully restore any errno that is set in pp_require while rifling
through @​INC trying to find the file being required (assuming the file
is eventually found and successfully opened).

But after it's opened, the file is sent to S_doeval to be compiled,
and somewhere in there errno gets cleared again. I haven't figured
out exactly how or where yet. There is a call to CLEAR_ERRSV(), which
has been there in some form since Perl 5.000. That might be what's
clearing errno, or not; I don't know. It doesn't look like it would,
but maybe there's magic on PL_errgv or something.

On the surface, it sounds reasonable that a successful require should
leave errno in the state it was in prior to the require. But nothing
resembling a complete or well-though-out implementation has been
proposed, and any change in this area will involve touching sensitive
bits of the core that have been as they are for twenty years. A
reformulated patch is attached, but at best it's a partial solution.

Thank you for your time and efforts on this.

pp_require is indeed quite involved and I fully understand anyone being
wary of touching it.

Still, according to the documentation, require dies on error, and
otherwise survives. So irrespective of the internal causes of changes to
$! and $^E, it should be OK to restore their values on entry in those
cases where require does not die. In Perl :

sub new_require {
my ($e, $se) = ($!, $^E);
old_require $_[0]; # dies on error...
($!, $^E) = ($e, $se); # ... and otherwise restore $! and $^E
}

While not strictly equivalent, and as an experiment, I wrapped the
existing pp_require, renamed pp_require1 but otherwise untouched, into a
new pp_require tasked with saving/restoring $! and $^E :

PP(pp_require1)
{
... existing code
}
PP(pp_require)
{
OP *op;
dSAVE_ERRNO;
op = Perl_pp_require1(
#ifndef PERL_IS_MINIPERL
my_perl
#endif
);
if ((*_errno()) == 0)
RESTORE_ERRNO;
return op;
}

However crude and ill-mannered, this hack seemed to work. A variation on
the above might be the best solution. A similar idea, implemented in the
attached patch, is to dSAVE_ERRNO as you did, but to RESTORE_ERRNO
before any return point that is not a DIE. I find four of those : two
RETPUSHYES, one RETPUSHUNDEF, and the final return. I can't see how
those RESTORE_ERRNOs could damage the working of pp_require, even with
the recursive calls that seem to occur (via S_doeval). Note that the
SETERRNO(0, SS_NORMAL) introduced in
<http​://perl5.git.perl.org/perl.git/commit/d8bfb8bddf933a815b590823bd52295534e6ded0?f=pp_ctl.c>
has been removed because 1) it seems unrelated to the internal mechanism
of pp_require, and 2) its effect would be nullified by the
RESTORE_ERRNOs (at least on success).

This seems to work (one.pm is successfully required, without clobbering
$! and $^E) :

blead(w32) $ echo 0 > zero.pm; echo 1 > one.pm
blead(w32) $ ../perl -IPerl -IRule -e 'for (@​ARGV) { $! = $^E = 1; eval
{require "$_"; 1} or print $@​; print $_,q{ ==&gt; $! = }, 0+$!, q{, $^E =
}, 0+$^E, qq{ [$^E]\n}}' doesnotexist.pm zero.pm one.pm
Can't locate doesnotexist.pm in @​INC (you may need to install the
doesnotexist module) (@​INC contains​: Perl Rule
D​:/perls/blead/perl-git2/lib .) at -e line 1.
doesnotexist.pm ==> $! = 2, $^E = 2 [Le fichier sp cifi est introuvable]
zero.pm did not return a true value at -e line 1.
zero.pm ==> $! = 1, $^E = 1 [Fonction incorrecte]
one.pm ==> $! = 1, $^E = 1 [Fonction incorrecte]
blead(w32) $

This is better than the stock perl (here strawberry perl portable 5.20)

Perl_5.20.0 $ echo 0 > zero.pm; echo 1 > one.pm
Perl_5.20.0 $ perl -IPerl -IRule -e 'for (@​ARGV) { $! = $^E = 1; eval
{require "$_"; 1} or print $@​; print $_,q{ ==&gt; $! = }, 0+$!, q{, $^E =
}, 0+$^E, qq{ [$^E]\n}}' doesnotexist.pm zero.pm one.pm
Can't locate doesnotexist.pm in @​INC (you may need to install the
doesnotexist module) (@​INC contains​: Perl Rule
C​:/strawberry-perl-5.20.0.1-32bit-portable-PDL/perl/site/lib/MSWin32-x86-multi-thread-64int
C​:/strawberry-perl-5.20.0.1-32bit-portable-PDL/perl/site/lib
C​:/strawberry-perl-5.20.0.1-32bit-portable-PDL/perl/vendor/lib
C​:/strawberry-perl-5.20.0.1-32bit-portable-PDL/perl/lib .) at -e line 1.
doesnotexist.pm ==> $! = 2, $^E = 2 [Le fichier spécifié est introuvable]
zero.pm did not return a true value at -e line 1.
zero.pm ==> $! = 0, $^E = 6 [Descripteur non valide]
one.pm ==> $! = 0, $^E = 6 [Descripteur non valide]
Perl_5.20.0 $

In case of failure the values of $! and $^E do not seem much worse with
the patch than without (given that for "did not return a true value"
they are not set anyway, so may be a bit random...).

Comments/critics/smokes warmly welcome :-) Best regards,

Christian

@p5pRT
Copy link
Author

p5pRT commented Jul 28, 2014

From cm.perl@abtela.com

0001-Restore-errno-on-succesfull-require-119555.patch
From ef0be7397327c777298a079d398b86062869b1b6 Mon Sep 17 00:00:00 2001
From: Christian Millour <cm.perl@abtela.com>
Date: Mon, 28 Jul 2014 23:34:22 +0200
Subject: [PATCH] Restore errno on succesfull require (#119555)

Historically, require used to clear errno ($!) on success. While errno
(and on some platforms $^E) might be set several times during a
require (such as when searching through @INC and failing on the first
directories), such temporary errors should not be visible to the user
when require finally succeeds. Clearing errno is wrong however, as
this produces pernicious action-at-a-distance effects (see #119555).
Not doing anything, as was the case with $^E, was no better as it left
$^E in a semi-random state. This patch addresses these issues by
saving $! and $^E on entry to require, and restoring their values on
successfull exit.
---
 pp_ctl.c |   19 +++++++++++--------
 1 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/pp_ctl.c b/pp_ctl.c
index 7d098b7..9975be2 100644
--- a/pp_ctl.c
+++ b/pp_ctl.c
@@ -3676,8 +3676,9 @@ PP(pp_require)
     SV *hook_sv = NULL;
     SV *encoding;
     OP *op;
-    int saved_errno;
+    int doopen_errno;
     bool path_searchable;
+    dSAVE_ERRNO;
 
     sv = POPs;
     if ( (SvNIOKp(sv) || SvVOK(sv)) && PL_op->op_type != OP_DOFILE) {
@@ -3735,6 +3736,7 @@ PP(pp_require)
 	    }
 	}
 
+	RESTORE_ERRNO;
 	RETPUSHYES;
     }
     name = SvPV_const(sv, len);
@@ -3777,9 +3779,10 @@ PP(pp_require)
 	SV * const * const svp = hv_fetch(GvHVn(PL_incgv),
 					  unixname, unixlen, 0);
 	if ( svp ) {
-	    if (*svp != &PL_sv_undef)
+	    if (*svp != &PL_sv_undef) {
+		RESTORE_ERRNO;
 		RETPUSHYES;
-	    else
+            } else
 		DIE(aTHX_ "Attempt to reload %s aborted.\n"
 			    "Compilation failed in require", unixname);
 	}
@@ -4022,13 +4025,13 @@ PP(pp_require)
 	    }
 	}
     }
-    saved_errno = errno; /* sv_2mortal can realloc things */
+    doopen_errno = errno; /* sv_2mortal can realloc things */
     sv_2mortal(namesv);
     if (!tryrsfp) {
 	if (PL_op->op_type == OP_REQUIRE) {
-	    if(saved_errno == EMFILE || saved_errno == EACCES) {
+	    if(doopen_errno == EMFILE || doopen_errno == EACCES) {
 		/* diag_listed_as: Can't locate %s */
-		DIE(aTHX_ "Can't locate %s:   %s", name, Strerror(saved_errno));
+		DIE(aTHX_ "Can't locate %s:   %s", name, Strerror(doopen_errno));
 	    } else {
 	        if (namesv) {			/* did we lookup @INC? */
 		    AV * const ar = GvAVn(PL_incgv);
@@ -4069,10 +4072,9 @@ PP(pp_require)
 	}
 
 	CLEAR_ERRSV();
+	RESTORE_ERRNO;
 	RETPUSHUNDEF;
     }
-    else
-	SETERRNO(0, SS_NORMAL);
 
     /* Assume success here to prevent recursive requirement. */
     /* name is never assigned to again, so len is still strlen(name)  */
@@ -4131,6 +4133,7 @@ PP(pp_require)
 
     LOADED_FILE_PROBE(unixname);
 
+    RESTORE_ERRNO;
     return op;
 }
 
-- 
1.7.4

@toddr
Copy link
Member

toddr commented Feb 14, 2020

This patch no longer applies. Does anyone else have an idea or do we call this a "feature" of Windows?

@toddr toddr added the Closable? We might be able to close this ticket, but we need to check with the reporter label Feb 14, 2020
@xenu xenu removed the affects-5.19 label Nov 19, 2021
@xenu xenu removed the Severity Low label Dec 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closable? We might be able to close this ticket, but we need to check with the reporter distro-mswin32 type-core
Projects
None yet
Development

No branches or pull requests

3 participants