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

Argument croak_on_error for eval_pv() is broken #17036

Closed
p5pRT opened this issue Jun 5, 2019 · 16 comments
Closed

Argument croak_on_error for eval_pv() is broken #17036

p5pRT opened this issue Jun 5, 2019 · 16 comments

Comments

@p5pRT
Copy link

p5pRT commented Jun 5, 2019

Migrated from rt.perl.org#134177 (status was 'pending release')

Searchable as RT134177$

@p5pRT
Copy link
Author

p5pRT commented Jun 5, 2019

From @pali

Here is test example which proves that when croak_on_error is set to 1
for eval_pv() function, it does not croak.

Just compile it, run and see that die from eval_pv with croak_on_error=1
does not throw any error.

$ xsubpp -prototypes -output test.c test.xs
$ gcc test.c -I`perl -MConfig -e 'print $Config​::Config{archlib}'`/CORE -fPIC -shared -o auto/test/test.so
$ perl -I. -Mtest -e ''
ERROR​: eval_pv not died
OK​: eval_pv died with $@​=string error at test.pm line 10.

Source test code is below​:

$ cat test.pm
package test;

use XSLoader;
BEGIN { XSLoader​::load(__PACKAGE__) }

use strict;
use warnings;

eval { eval_pv 'die bool_obj->new(0)'; 1 } and do { print "ERROR​: eval_pv not died\n"; 1; } or do { print "OK​: eval_pv died with \$@​=$@​" };
eval { eval_pv 'die bool_obj->new(1)'; 1 } and do { print "ERROR​: eval_pv not died\n"; 1; } or do { print "OK​: eval_pv died with \$@​=$@​" };

package bool_obj;
use overload 'bool' => sub { ${$_[0]} };
use overload '""' => sub { "string error" };
sub new { bless \do{my $tmp = $_[1]}, $_[0] }

1;

$ cat test.xs
#define PERL_NO_GET_CONTEXT
#include "EXTERN.h"
#include "perl.h"
#include "XSUB.h"

MODULE = test PACKAGE = test

void
eval_pv(string, croak_on_error = 1)
  const char *string
  I32 croak_on_error
PPCODE​:
  EXTEND(SP, 1);
  PUSHs(eval_pv(string, croak_on_error));

@p5pRT
Copy link
Author

p5pRT commented Jun 14, 2019

From @pali

What about checking for SvROK? Or installing __DIE__ handler and
checking if __DIE__ handler was called?

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2019

From @tonycoz

On Fri, 14 Jun 2019 00​:44​:07 -0700, pali@​cpan.org wrote​:

What about checking for SvROK? Or installing __DIE__ handler and
checking if __DIE__ handler was called?

When I suggested using ROK in #p5p you brought up clobbering $@​ as a problem, but I don't think it is one - eval in all its forms clobbers $@​ and I don't think it's unreasonable to expect that from eval_pv()/eval_sv() (and eval_sv() can G_KEEPERR).

Using a __DIE__ handler would be unnecessarily complex - you'd be better off modifying eval_sv()[1] to accept a G_RETHROW flag, which wouldn't be too hard.

Is there some way changing​:

  if(SvTRUE_NN(errsv))

to

  if(SvTRUE_NN(errsv) || SvROK(errsv))

doesn't solve this?

Tony

[1] G_EVALBYTE and G_EVALUNICODE to match OPpEVAL_BYTES and OPpEVAL_UNICODE might also be useful.

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2019

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

@p5pRT
Copy link
Author

p5pRT commented Jun 17, 2019

From @pali

On Sunday 16 June 2019 21​:40​:43 Tony Cook via RT wrote​:

On Fri, 14 Jun 2019 00​:44​:07 -0700, pali@​cpan.org wrote​:

What about checking for SvROK? Or installing __DIE__ handler and
checking if __DIE__ handler was called?

When I suggested using ROK in #p5p you brought up clobbering $@​ as a problem, but I don't think it is one - eval in all its forms clobbers $@​ and I don't think it's unreasonable to expect that from eval_pv()/eval_sv() (and eval_sv() can G_KEEPERR).

Using a __DIE__ handler would be unnecessarily complex - you'd be better off modifying eval_sv()[1] to accept a G_RETHROW flag, which wouldn't be too hard.

Seems like introduction of G_RETHROW in eval_sv() is the best way.

Is there some way changing​:

if(SvTRUE_NN(errsv))

to

if(SvTRUE_NN(errsv) || SvROK(errsv))

Rather check SvROK before SvTRUE

  if(SvROK(errsv) || SvTRUE_NN(errsv))

as SvTRUE would call object overloaded method. And it is useless.

doesn't solve this?

So is clobbering of $@​ in eval_sv() possible or not?

Tony

[1] G_EVALBYTE and G_EVALUNICODE to match OPpEVAL_BYTES and OPpEVAL_UNICODE might also be useful.

@p5pRT
Copy link
Author

p5pRT commented Jun 20, 2019

From @tonycoz

On Mon, 17 Jun 2019 01​:06​:58 -0700, pali@​cpan.org wrote​:

On Sunday 16 June 2019 21​:40​:43 Tony Cook via RT wrote​:

On Fri, 14 Jun 2019 00​:44​:07 -0700, pali@​cpan.org wrote​:

What about checking for SvROK? Or installing __DIE__ handler and
checking if __DIE__ handler was called?

When I suggested using ROK in #p5p you brought up clobbering $@​ as a
problem, but I don't think it is one - eval in all its forms clobbers
$@​ and I don't think it's unreasonable to expect that from
eval_pv()/eval_sv() (and eval_sv() can G_KEEPERR).

Using a __DIE__ handler would be unnecessarily complex - you'd be
better off modifying eval_sv()[1] to accept a G_RETHROW flag, which
wouldn't be too hard.

Seems like introduction of G_RETHROW in eval_sv() is the best way.

See the attached.

Is there some way changing​:

if(SvTRUE_NN(errsv))

to

if(SvTRUE_NN(errsv) || SvROK(errsv))

Rather check SvROK before SvTRUE

if(SvROK(errsv) || SvTRUE_NN(errsv))

as SvTRUE would call object overloaded method. And it is useless.

Not that it matters if the attached is applied, but the checks SvTRUE() does seem more likely to matter than the SvROK() check in most common cases.

doesn't solve this?

So is clobbering of $@​ in eval_sv() possible or not?

Evals called within whatever code eval_sv() calls can still clobber $@​.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 20, 2019

From @tonycoz

0001-perl-134177-add-G_RETHROW-flag-to-eval_sv.patch
From f79274795bd1fbd4808a731648ea158a964a14de Mon Sep 17 00:00:00 2001
From: Tony Cook <tony@develop-help.com>
Date: Thu, 20 Jun 2019 15:26:22 +1000
Subject: (perl #134177) add G_RETHROW flag to eval_sv()

and update eval_pv() to use it.
---
 cop.h                      |  1 +
 ext/XS-APItest/Makefile.PL |  2 +-
 ext/XS-APItest/t/call.t    | 26 +++++++++++++++++++++++++-
 perl.c                     | 25 ++++++++++++++++---------
 4 files changed, 43 insertions(+), 11 deletions(-)

diff --git a/cop.h b/cop.h
index fca2aa755c..9cf85ef883 100644
--- a/cop.h
+++ b/cop.h
@@ -973,6 +973,7 @@ L<perlcall>.
 				    Perl_magic_methcall().  */
 #define G_RE_REPARSING 0x800     /* compiling a run-time /(?{..})/ */
 #define G_METHOD_NAMED 4096	/* calling named method, eg without :: or ' */
+#define G_RETHROW      8192	/* eval_sv(): re-throw any error */
 
 /* flag bits for PL_in_eval */
 #define EVAL_NULL	0	/* not in an eval */
diff --git a/ext/XS-APItest/Makefile.PL b/ext/XS-APItest/Makefile.PL
index d79ba1150e..3fe5e397a8 100644
--- a/ext/XS-APItest/Makefile.PL
+++ b/ext/XS-APItest/Makefile.PL
@@ -23,7 +23,7 @@ WriteMakefile(
 my @names = (qw(HV_DELETE HV_DISABLE_UVAR_XKEY HV_FETCH_ISSTORE
 		HV_FETCH_ISEXISTS HV_FETCH_LVALUE HV_FETCH_JUST_SV
 		G_SCALAR G_ARRAY G_VOID G_DISCARD G_EVAL G_NOARGS
-		G_KEEPERR G_NODEBUG G_METHOD G_FAKINGEVAL
+		G_KEEPERR G_NODEBUG G_METHOD G_FAKINGEVAL G_RETHROW
 		GV_NOADD_NOINIT
 		IS_NUMBER_IN_UV IS_NUMBER_GREATER_THAN_UV_MAX
 		IS_NUMBER_NOT_INT IS_NUMBER_NEG IS_NUMBER_INFINITY
diff --git a/ext/XS-APItest/t/call.t b/ext/XS-APItest/t/call.t
index 632a421d4f..e4228077cb 100644
--- a/ext/XS-APItest/t/call.t
+++ b/ext/XS-APItest/t/call.t
@@ -11,7 +11,7 @@ use strict;
 
 BEGIN {
     require '../../t/test.pl';
-    plan(530);
+    plan(538);
     use_ok('XS::APItest')
 };
 
@@ -228,6 +228,30 @@ is(eval { eval_pv(q/die $obj/, 1) }, undef,
 ok(ref $@, "object thrown");
 is($@, $obj, "check object rethrown");
 
+package False {
+    use overload
+      bool => sub { 0 },
+      '""' => sub { "Foo" };
+    sub new { bless {}, shift }
+};
+my $false = False->new;
+ok(!$false, "our false object is actually false");
+is(eval { eval_pv(q/die $false;/, 1); 1 }, undef,
+   "check false objects are rethrown");
+is(overload::StrVal($@), overload::StrVal($false),
+   "check we got the expected object");
+
+is(eval { eval_sv(q/die $false/, G_RETHROW); 1 }, undef,
+   "check G_RETHROW for thrown object");
+is(overload::StrVal($@), overload::StrVal($false),
+   "check we got the expected object");
+is(eval { eval_sv(q/"unterminated/, G_RETHROW); 1 }, undef,
+   "check G_RETHROW for syntax error");
+like($@, qr/Can't find string terminator/,
+     "check error rethrown");
+ok(eq_array([ eval { eval_sv(q/"working code"/, G_RETHROW) } ], [ "working code", 1 ]),
+   "check for spurious rethrow");
+
 # #3719 - check that the eval call variants handle exceptions correctly,
 # and do the right thing with $@, both with and without G_KEEPERR set.
 
diff --git a/perl.c b/perl.c
index e71ecaf8fc..d851099a1d 100644
--- a/perl.c
+++ b/perl.c
@@ -3101,6 +3101,9 @@ Perl_call_sv(pTHX_ SV *sv, volatile I32 flags)
 Tells Perl to C<eval> the string in the SV.  It supports the same flags
 as C<call_sv>, with the obvious exception of C<G_EVAL>.  See L<perlcall>.
 
+The C<G_RETHROW> flag can be used if you only need eval_sv() to
+execute code specified by a string, but not catch any errors.
+
 =cut
 */
 
@@ -3182,6 +3185,11 @@ Perl_eval_sv(pTHX_ SV *sv, I32 flags)
 	    goto redo_body;
 	}
       fail:
+        if (flags & G_RETHROW) {
+            JMPENV_POP;
+            croak_sv(ERRSV);
+        }
+
 	PL_stack_sp = PL_stack_base + oldmark;
 	if ((flags & G_WANT) == G_ARRAY)
 	    retval = 0;
@@ -3218,8 +3226,14 @@ Perl_eval_pv(pTHX_ const char *p, I32 croak_on_error)
 
     PERL_ARGS_ASSERT_EVAL_PV;
 
-    eval_sv(sv, G_SCALAR);
-    SvREFCNT_dec(sv);
+    if (croak_on_error) {
+        sv_2mortal(sv);
+        eval_sv(sv, G_SCALAR | G_RETHROW);
+    }
+    else {
+        eval_sv(sv, G_SCALAR);
+        SvREFCNT_dec(sv);
+    }
 
     {
         dSP;
@@ -3227,13 +3241,6 @@ Perl_eval_pv(pTHX_ const char *p, I32 croak_on_error)
         PUTBACK;
     }
 
-    /* just check empty string or undef? */
-    if (croak_on_error) {
-	SV * const errsv = ERRSV;
-	if(SvTRUE_NN(errsv))
-            croak_sv(errsv);
-    }
-
     return sv;
 }
 
-- 
2.11.0

@p5pRT
Copy link
Author

p5pRT commented Jun 20, 2019

From @pali

On Wednesday 19 June 2019 22​:31​:43 Tony Cook via RT wrote​:

On Mon, 17 Jun 2019 01​:06​:58 -0700, pali@​cpan.org wrote​:

On Sunday 16 June 2019 21​:40​:43 Tony Cook via RT wrote​:

On Fri, 14 Jun 2019 00​:44​:07 -0700, pali@​cpan.org wrote​:

What about checking for SvROK? Or installing __DIE__ handler and
checking if __DIE__ handler was called?

When I suggested using ROK in #p5p you brought up clobbering $@​ as a
problem, but I don't think it is one - eval in all its forms clobbers
$@​ and I don't think it's unreasonable to expect that from
eval_pv()/eval_sv() (and eval_sv() can G_KEEPERR).

Using a __DIE__ handler would be unnecessarily complex - you'd be
better off modifying eval_sv()[1] to accept a G_RETHROW flag, which
wouldn't be too hard.

Seems like introduction of G_RETHROW in eval_sv() is the best way.

See the attached.

Looks good, thanks!

Is there some way changing​:

if(SvTRUE_NN(errsv))

to

if(SvTRUE_NN(errsv) || SvROK(errsv))

Rather check SvROK before SvTRUE

if(SvROK(errsv) || SvTRUE_NN(errsv))

as SvTRUE would call object overloaded method. And it is useless.

Not that it matters if the attached is applied, but the checks SvTRUE() does seem more likely to matter than the SvROK() check in most common cases.

Yes, it does not matter for patch which is attached, but SvROK() just
checks sv flags which is fast, but SvTRUE() is slow and has side
effects.

doesn't solve this?

So is clobbering of $@​ in eval_sv() possible or not?

Evals called within whatever code eval_sv() calls can still clobber $@​.

I tried to play with it, but I was not able to write any code which
would clobber $@​. And provide incorrect result with SvROK + SvTRUE
checks. So it is really possible?

@p5pRT
Copy link
Author

p5pRT commented Jun 20, 2019

From @tonycoz

On Thu, Jun 20, 2019 at 10​:18​:40AM +0200, pali@​cpan.org wrote​:

doesn't solve this?

So is clobbering of $@​ in eval_sv() possible or not?

Evals called within whatever code eval_sv() calls can still clobber $@​.

I tried to play with it, but I was not able to write any code which
would clobber $@​. And provide incorrect result with SvROK + SvTRUE
checks. So it is really possible?

It has no effect on that, I meant that​:

./perl -Ilib -MXS​::APItest -le '$@​ = "foo"; eval_sv("eval q/1/", G_KEEPERR); print $@​'

$@​ isn't preserved.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 20, 2019

From @pali

On Thursday 20 June 2019 02​:49​:00 Tony Cook via RT wrote​:

On Thu, Jun 20, 2019 at 10​:18​:40AM +0200, pali@​cpan.org wrote​:

doesn't solve this?

So is clobbering of $@​ in eval_sv() possible or not?

Evals called within whatever code eval_sv() calls can still clobber $@​.

I tried to play with it, but I was not able to write any code which
would clobber $@​. And provide incorrect result with SvROK + SvTRUE
checks. So it is really possible?

It has no effect on that, I meant that​:

./perl -Ilib -MXS​::APItest -le '$@​ = "foo"; eval_sv("eval q/1/", G_KEEPERR); print $@​'

$@​ isn't preserved.

I mean $@​ clobbering in eval_pv(). E.g. when eval_pv() is called with
croak_on_error=1.

@p5pRT
Copy link
Author

p5pRT commented Jun 24, 2019

From @tonycoz

On Thu, 20 Jun 2019 02​:51​:32 -0700, pali@​cpan.org wrote​:

On Thursday 20 June 2019 02​:49​:00 Tony Cook via RT wrote​:

On Thu, Jun 20, 2019 at 10​:18​:40AM +0200, pali@​cpan.org wrote​:

doesn't solve this?

So is clobbering of $@​ in eval_sv() possible or not?

Evals called within whatever code eval_sv() calls can still
clobber $@​.

I tried to play with it, but I was not able to write any code which
would clobber $@​. And provide incorrect result with SvROK + SvTRUE
checks. So it is really possible?

It has no effect on that, I meant that​:

./perl -Ilib -MXS​::APItest -le '$@​ = "foo"; eval_sv("eval q/1/",
G_KEEPERR); print $@​'

$@​ isn't preserved.

I mean $@​ clobbering in eval_pv(). E.g. when eval_pv() is called with
croak_on_error=1.

eval_pv() never uses (and never will for compatibility) the G_KEEPERR flag when calling eval_sv(), so it will always clobber $@​.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jun 24, 2019

From @pali

On Sunday 23 June 2019 18​:42​:34 Tony Cook via RT wrote​:

On Thu, 20 Jun 2019 02​:51​:32 -0700, pali@​cpan.org wrote​:

On Thursday 20 June 2019 02​:49​:00 Tony Cook via RT wrote​:

On Thu, Jun 20, 2019 at 10​:18​:40AM +0200, pali@​cpan.org wrote​:

doesn't solve this?

So is clobbering of $@​ in eval_sv() possible or not?

Evals called within whatever code eval_sv() calls can still
clobber $@​.

I tried to play with it, but I was not able to write any code which
would clobber $@​. And provide incorrect result with SvROK + SvTRUE
checks. So it is really possible?

It has no effect on that, I meant that​:

./perl -Ilib -MXS​::APItest -le '$@​ = "foo"; eval_sv("eval q/1/",
G_KEEPERR); print $@​'

$@​ isn't preserved.

I mean $@​ clobbering in eval_pv(). E.g. when eval_pv() is called with
croak_on_error=1.

eval_pv() never uses (and never will for compatibility) the G_KEEPERR flag when calling eval_sv(), so it will always clobber $@​.

I tried to create any other examples where eval_pv() with
croak_on_error=1 would not work correctly. But I was not able to come up
with any other.

All following examples works correctly, no error/fail.

eval { eval_pv '$@​ = "croak"'; 1 } and do { print "OK​: eval_pv not died, \$@​=$@​\n"; 1 } or do { print "ERROR​: eval_pv died with \$@​=$@​" };
eval { eval_pv 'eval { die "croak"; }'; 1 } and do { print "OK​: eval_pv not died, \$@​=$@​\n"; 1 } or do { print "ERROR​: eval_pv died with \$@​=$@​" };
eval { eval_pv 'sub obj​::DESTROY { eval { die "croak" } } my $obj = bless {}, "obj";'; 1 } and do { print "OK​: eval_pv not died, \$@​=$@​\n"; 1 } or do { print "ERROR​: eval_pv died with \$@​=$@​" };
eval { eval_pv 'local $@​; die "message"'; 1 } and do { print "ERROR​: eval_pv not died, \$@​=$@​\n"; 1 } or do { print "OK​: eval_pv died with \$@​=$@​" };

So how can *unpatched* eval_pv() clobbers $@​ and make croak_on_error=1
to behave incorrectly? The only example which I created was that one
with overloading 'bool' object method to false.

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2019

From @pali

On Wednesday 19 June 2019 22​:31​:43 Tony Cook via RT wrote​:

On Mon, 17 Jun 2019 01​:06​:58 -0700, pali@​cpan.org wrote​:

On Sunday 16 June 2019 21​:40​:43 Tony Cook via RT wrote​:

On Fri, 14 Jun 2019 00​:44​:07 -0700, pali@​cpan.org wrote​:

What about checking for SvROK? Or installing __DIE__ handler and
checking if __DIE__ handler was called?

When I suggested using ROK in #p5p you brought up clobbering $@​ as a
problem, but I don't think it is one - eval in all its forms clobbers
$@​ and I don't think it's unreasonable to expect that from
eval_pv()/eval_sv() (and eval_sv() can G_KEEPERR).

Using a __DIE__ handler would be unnecessarily complex - you'd be
better off modifying eval_sv()[1] to accept a G_RETHROW flag, which
wouldn't be too hard.

Seems like introduction of G_RETHROW in eval_sv() is the best way.

See the attached.

Hi! Have you applied this G_RETHROW patch?

@p5pRT
Copy link
Author

p5pRT commented Jul 8, 2019

From @tonycoz

On Thu, 04 Jul 2019 03​:11​:45 -0700, pali@​cpan.org wrote​:

On Wednesday 19 June 2019 22​:31​:43 Tony Cook via RT wrote​:

On Mon, 17 Jun 2019 01​:06​:58 -0700, pali@​cpan.org wrote​:

On Sunday 16 June 2019 21​:40​:43 Tony Cook via RT wrote​:

On Fri, 14 Jun 2019 00​:44​:07 -0700, pali@​cpan.org wrote​:

What about checking for SvROK? Or installing __DIE__ handler and
checking if __DIE__ handler was called?

When I suggested using ROK in #p5p you brought up clobbering $@​ as a
problem, but I don't think it is one - eval in all its forms clobbers
$@​ and I don't think it's unreasonable to expect that from
eval_pv()/eval_sv() (and eval_sv() can G_KEEPERR).

Using a __DIE__ handler would be unnecessarily complex - you'd be
better off modifying eval_sv()[1] to accept a G_RETHROW flag, which
wouldn't be too hard.

Seems like introduction of G_RETHROW in eval_sv() is the best way.

See the attached.

Hi! Have you applied this G_RETHROW patch?

Done in fb81daf, 66ff4fb.

Thanks for your feedback.

Tony

@p5pRT
Copy link
Author

p5pRT commented Jul 8, 2019

@tonycoz - Status changed from 'open' to 'pending release'

@p5pRT p5pRT closed this as completed Jul 8, 2019
@p5pRT
Copy link
Author

p5pRT commented Jul 8, 2019

From @pali

On Sunday 07 July 2019 18​:57​:12 Tony Cook via RT wrote​:

On Thu, 04 Jul 2019 03​:11​:45 -0700, pali@​cpan.org wrote​:

On Wednesday 19 June 2019 22​:31​:43 Tony Cook via RT wrote​:

On Mon, 17 Jun 2019 01​:06​:58 -0700, pali@​cpan.org wrote​:

On Sunday 16 June 2019 21​:40​:43 Tony Cook via RT wrote​:

On Fri, 14 Jun 2019 00​:44​:07 -0700, pali@​cpan.org wrote​:

What about checking for SvROK? Or installing __DIE__ handler and
checking if __DIE__ handler was called?

When I suggested using ROK in #p5p you brought up clobbering $@​ as a
problem, but I don't think it is one - eval in all its forms clobbers
$@​ and I don't think it's unreasonable to expect that from
eval_pv()/eval_sv() (and eval_sv() can G_KEEPERR).

Using a __DIE__ handler would be unnecessarily complex - you'd be
better off modifying eval_sv()[1] to accept a G_RETHROW flag, which
wouldn't be too hard.

Seems like introduction of G_RETHROW in eval_sv() is the best way.

See the attached.

Hi! Have you applied this G_RETHROW patch?

Done in fb81daf, 66ff4fb.

Thanks for your feedback.

Thanks!

Now reminds only my question about clobbering​:
https://rt.perl.org/Public/Bug/Display.html?id=134177#txn-1642800
Can version without G_RETHROW really breaks croak_on_error=1?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant