Skip to content

Safe.pm sort {} bug with -Dusethreads #9557

Closed
Closed
Safe.pm sort {} bug with -Dusethreads#9557

Description

@p5pRT
p5pRT
opened on Nov 6, 2008

Migrated from rt.perl.org#60374 (status was 'resolved')

Searchable as RT60374$

Activity

  1. p5pRT commented on Nov 6, 2008

    @p5pRT
    p5pRT
    on Nov 6, 2008
    Author

    From badalex@gmail.com

    When using a threading perl and you reval an anon sub that contains a sort,
    a/$b loose their values. Both 5.8.8 and 5.10.0 seem to be affected.

    (see attached perl.v for perl -V output)
    -Dusethreads
    perl tsafe.pl
    debug​:
    debug​:
    debug​:
    debug​:
    debug​:
    debug​:
    debug​:
    debug​:
    debug​: 5 4 3 2 1

    without -Dusethreads
    perl1 tsafe.pl
    debug​: 4
    debug​: 2
    debug​: 2
    debug​: 3
    debug​: 1
    debug​: 1 2 3 4 5

    tsafe.pl

    use Safe;

    my $safe = Safe->new('PLPerl');
    $safe->permit_only('​:default');
    $safe->permit(qw(sort));
    $safe->share(qw(&debug));

    sub debug {
      print "debug​: ". (shift) . "\n";
    }

    my $func = $safe->reval(<<'z');
    sub {
    my @​c = sort { debug("$b"); "$a" <=> $b } qw(5 4 3 2 1);
    debug(join(" ", @​c));
    return;
    }
    z

    #my $f = eval 'package PLPerl; sub { @​_=(); $func->(); }';
    $func->();

    die $@​ if($@​);

  2. p5pRT commented on Nov 6, 2008

    @p5pRT
    p5pRT
    on Nov 6, 2008
    Author

    From badalex@gmail.com

    perl.v

  3. p5pRT commented on Nov 6, 2008

    @p5pRT
    p5pRT
    on Nov 6, 2008
    Author

    From badalex@gmail.com

    perl1.v

  4. p5pRT commented on Nov 6, 2008

    @p5pRT
    p5pRT
    on Nov 6, 2008
    Author

    From badalex@gmail.com

    tsafe.pl

  5. p5pRT commented on Nov 6, 2008

    @p5pRT
    p5pRT
    on Nov 6, 2008
    Author

    From badalex@gmail.com

    Andrew Gierth <andrew@​tao11.riddles.org.uk> managed to track it down
    his explanation is below​:

    If it helps any, I've tracked down in the perl guts exactly why this
    happens​:

    cop.h​:

    struct cop {
      BASEOP
      char * cop_label; /* label for this construct */
    #ifdef USE_ITHREADS
      char * cop_stashpv; /* package line was compiled in */
      char * cop_file; /* file name the following line # is from */
    #else
      HV * cop_stash; /* package line was compiled in */
      GV * cop_filegv; /* file the following line # is from */
    #endif
      U32 cop_seq; /* parse sequence number */
      I32 cop_arybase; /* array base this line was compiled with */
      line_t cop_line; /* line # of this command */
      SV * cop_warnings; /* lexical warnings bitmask */
      SV * cop_io; /* lexical IO defaults */
    };

    A COP in perl is a control operation, basically a compiled statement,
    and the pointer to the current COP is used to determine all the
    lexical state, including the current package. pp_sort uses
    CopSTASH(PL_curcop) to get the package stash (symbol table) in order
    to locate the $a and $b variables in it.

    Notice, though, that without ithreads, the COP points directly to the
    stash, but with ithreads, it points instead to the _name_ of the stash
    (e.g. "main"). The problem arises because with Safe in use, the
    package created by Safe to use as a container _thinks_ that its name
    is "main" even though it's not, so the COPs compiled inside it point
    to the name "main" rather than to the real name of the container.

    So with ithreads enabled, pp_sort looks up the package stash by name,
    gets the "main" package rather than the safe container, and creates
    $main​::a and $main​::b to store the comparison values in. But the
    compiled comparison block has its own references to the variables
    which refers to the correct stash, so it all goes Horribly Wrong at
    that point.

    So there are three factors involved​:

    1) the change in layout of COP with ithreads enabled
    2) the fact that Safe changes the internally-seen name of a package
    3) any operation that relies on CopSTASH(PL_curcop) (I can only find a
    few​: sort, reset, and bless) will then behave incorrectly

    However, I have no idea why Perl has this difference between threaded
    and non-threaded code.

  6. p5pRT commented on Nov 6, 2008

    @p5pRT
    p5pRT
    on Nov 6, 2008
    Author

    badalex@gmail.com - Status changed from 'new' to 'open'

  7. p5pRT commented on Nov 8, 2008

    @p5pRT
    p5pRT
    on Nov 8, 2008
    Author

    From badalex@gmail.com

    Andrew Gierth <andrew@​tao11.riddles.org.uk> managed to track it down
    his explanation is below​:

    If it helps any, I've tracked down in the perl guts exactly why this
    happens​:

    cop.h​:

    struct cop {
      BASEOP
      char * cop_label; /* label for this construct */
    #ifdef USE_ITHREADS
      char * cop_stashpv; /* package line was compiled in */
      char * cop_file; /* file name the following line # is from */
    #else
      HV * cop_stash; /* package line was compiled in */
      GV * cop_filegv; /* file the following line # is from */
    #endif
      U32 cop_seq; /* parse sequence number */
      I32 cop_arybase; /* array base this line was compiled with */
      line_t cop_line; /* line # of this command */
      SV * cop_warnings; /* lexical warnings bitmask */
      SV * cop_io; /* lexical IO defaults */
    };

    A COP in perl is a control operation, basically a compiled statement,
    and the pointer to the current COP is used to determine all the
    lexical state, including the current package. pp_sort uses
    CopSTASH(PL_curcop) to get the package stash (symbol table) in order
    to locate the $a and $b variables in it.

    Notice, though, that without ithreads, the COP points directly to the
    stash, but with ithreads, it points instead to the _name_ of the stash
    (e.g. "main"). The problem arises because with Safe in use, the
    package created by Safe to use as a container _thinks_ that its name
    is "main" even though it's not, so the COPs compiled inside it point
    to the name "main" rather than to the real name of the container.

    So with ithreads enabled, pp_sort looks up the package stash by name,
    gets the "main" package rather than the safe container, and creates
    $main​::a and $main​::b to store the comparison values in. But the
    compiled comparison block has its own references to the variables
    which refers to the correct stash, so it all goes Horribly Wrong at
    that point.

    So there are three factors involved​:

    1) the change in layout of COP with ithreads enabled
    2) the fact that Safe changes the internally-seen name of a package
    3) any operation that relies on CopSTASH(PL_curcop) (I can only find a
      few​: sort, reset, and bless) will then behave incorrectly

    However, I have no idea why Perl has this difference between threaded
    and non-threaded code.

  8. p5pRT commented on Oct 6, 2009

    @p5pRT
    p5pRT
    on Oct 6, 2009
    Author

    From @timbunce

    Any news of progress with this bug?

    Meanwhile, I have a workaround​: explicitly share the globs for a and b. In other words, add *a
    and * b to the arguments of the share() method​:

    $safe->share(qw(&debug *a *b));

  9. p5pRT commented on Oct 7, 2009

    @p5pRT
    p5pRT
    on Oct 7, 2009
    Author

    From badalex@gmail.com

    On Tue, Oct 6, 2009 at 13​:55, Tim Bunce via RT
    <perlbug-followup@​perl.org> wrote​:

    Any news of progress with this bug?

    Nope still broke for me on 5.10.1

    Here are the workaround I know about​:
    1) dont use $a and $b in sort, use @​_
    2) your way of adding *a and *b, but that only works as long as the
    package stays the same...
    3) declare the package at the top of reval, i.e.
      my $safe = Safe->new('PLPerl');
      $safe->permit(qw(sort));
      my $func = $safe->reval(<<'eval');
      package PLPerl;
      sub {
      sort { "$a" cmp "$b" } qw(works now);
      }
      eval

      $func->();

    4) fix perl :) as mentioned in the previous message

    Meanwhile, I have a workaround​: explicitly share the globs for a and b. In other words, add *a
    and * b to the arguments of the share() method​:

    $safe->share(qw(&debug *a *b));

    Yeah as noted above that should work. Unless you ever sort inside a
    different package than you gave to Safe->new() (see the below test
    case)

    use Safe();

    my $safe = Safe->new('PLPerl');
    $safe->permit_only('​:default');
    $safe->permit(qw(sort));
    $safe->share(qw(&PLPerl​::debug *a *b));

    my $func = $safe->reval(<<'z');
    sub {
    package PLPrl;
    my @​c = sort { PLPerl​::debug("$a ". $b); "$a" <=> $b } qw(5 4 3 2 1);
    PLPerl​::debug(join(" ", @​c));
    return;
    }
    z

    $func->();

    package PLPerl;
    sub debug {
      print "debug​: ". (shift) . "\n";
    }

    $ perl tsafe.pl
    debug​:
    debug​:
    debug​:
    debug​:
    debug​:
    debug​:
    debug​:
    debug​:
    debug​: 5 4 3 2 1

  10. p5pRT commented on Oct 7, 2009

    @p5pRT
    p5pRT
    on Oct 7, 2009
    Author

    From @rgarcia

    2009/10/6 Tim Bunce via RT <perlbug-followup@​perl.org>​:

    Any news of progress with this bug?

    Meanwhile, I have a workaround​: explicitly share the globs for a and b. In other words, add *a
    and * b to the arguments of the share() method​:

    $safe->share(qw(&debug *a *b));

    I don't think that *a and *b should be added to the default sharelist
    of Safe. Actually the only glob there is *_, and I think it should
    remain as is.

    The bug is most likely in the sort implementation, which is
    notoriously thread-unsafe when it invokes sort subs.

    In short, my advice would be : don't use sort() at all with a threaded perl.

  11. p5pRT commented on Oct 7, 2009

    @p5pRT
    p5pRT
    on Oct 7, 2009
    Author

    From david@kineticode.com

    On Oct 7, 2009, at 8​:22 AM, Rafael Garcia-Suarez wrote​:

    The bug is most likely in the sort implementation, which is
    notoriously thread-unsafe when it invokes sort subs.

    In short, my advice would be : don't use sort() at all with a
    threaded perl.

    Say what? Sure, on current releases, but surely that should be fixed.
    There are more and more threaded Perls every day…

    David

  12. p5pRT commented on Oct 7, 2009

    @p5pRT
    p5pRT
    on Oct 7, 2009
    Author

    From badalex@gmail.com

    On Wed Oct 07 08​:22​:57 2009, rgs@​consttype.org wrote​:

    The bug is most likely in the sort implementation, which is
    notoriously thread-unsafe when it invokes sort subs.

    In short, my advice would be : don't use sort() at all with a threaded
    perl.

    Um... you do realize its just a threaded *build* of perl. Nothing here
    is actually threaded or even using threads... Right?

  13. p5pRT commented on Oct 7, 2009

    @p5pRT
    p5pRT
    on Oct 7, 2009
    Author

    From @rgarcia

    2009/10/7 David E. Wheeler <david@​kineticode.com>​:

    On Oct 7, 2009, at 8​:22 AM, Rafael Garcia-Suarez wrote​:

    The bug is most likely in the sort implementation, which is
    notoriously thread-unsafe when it invokes sort subs.

    In short, my advice would be : don't use sort() at all with a threaded
    perl.

    Say what? Sure, on current releases, but surely that should be fixed. There
    are more and more threaded Perls every day…

    Even if we manage someone to fix threading, this will slow down
    sort(). Threading also has a non-negligeable performance impact on
    memory and speed of every perl program.

    I think we should advice people in INSTALL to build with threading
    only if they really need it, and tell OS vendors to disable threading
    on their system perls.

  14. p5pRT commented on Oct 7, 2009

    @p5pRT
    p5pRT
    on Oct 7, 2009
    Author

    From david@kineticode.com

    On Oct 7, 2009, at 10​:24 AM, Rafael Garcia-Suarez wrote​:

    Even if we manage someone to fix threading, this will slow down
    sort(). Threading also has a non-negligeable performance impact on
    memory and speed of every perl program.

    I think we should advice people in INSTALL to build with threading
    only if they really need it, and tell OS vendors to disable threading
    on their system perls.

    Snow Leopard ships with a threaded Perl. Hell, I think Leopard did.
    That cat might have left the bag.

    Best,

    David

  15. p5pRT commented on Oct 8, 2009

    @p5pRT
    p5pRT
    on Oct 8, 2009
    Author

    From @jandubois

    On Wed, 07 Oct 2009, Rafael Garcia-Suarez wrote​:

    The bug is most likely in the sort implementation, which is
    notoriously thread-unsafe when it invokes sort subs.

    In short, my advice would be : don't use sort() at all with a threaded perl.

    Just to be sure​: That advice is in the context of using Safe.pm,
    right? Or are there problems with sort() ourside safe compartments
    under threading as well? Are there any open bugs about them?

    Cheers,
    -Jan

  16. 20 remaining items

  17. p5pRT commented on Nov 7, 2009

    @p5pRT
    p5pRT
    on Nov 7, 2009
    Author

    From badalex@gmail.com

    On Fri, Nov 6, 2009 at 03​:44, Tim Bunce via RT
    <perlbug-followup@​perl.org> wrote​:

    On Tue, Oct 06, 2009 at 12​:55​:47PM -0700, Tim Bunce via RT wrote​:

    Any news of progress with this bug?

    To summarize the thread so far...
    - it's confirmed that the bug is still present in 5.10.1.
    - we digressed into a "don't use threaded perl" discussion
       but that's not an option for many people these days.

    Yes, and thanks for pointing out that the above really has nothing to
    do with *this* bug. Its not running a sort with threads its just
    broken with a threaded *build* of perl. :)

    - some workarounds are possible, but *none work* for calls to
       sort() with custom sort subs in third-party code

    Um the last patch I posted should...
    http​://rt.perl.org/rt3/Ticket/Display.html?id=60374#txn-612980. And
    while I found it quite ugly and im sure there are better ways to fix
    this... (namely we could fix Opcode​::_safe_call_sv to do something
    similar, or fix it as diagnosed in
    http​://rt.perl.org/rt3/Ticket/Display.html?id=60374#txn-491794) I was
    more or less looking for some kind of validation as to the approach
    :).

    People seemed to prefer to talk about how you should never be using a
    threaded perl anyways... *shrug* Perhaps I should submit a patch to
    disable -DUSETHREADS instead ?

  18. p5pRT commented on Nov 8, 2009

    @p5pRT
    p5pRT
    on Nov 8, 2009
    Author

    From @timbunce

    On Fri, Nov 06, 2009 at 02​:30​:47PM -0700, Alex Hunsaker wrote​:

    Um the last patch I posted should...
    http​://rt.perl.org/rt3/Ticket/Display.html?id=60374#txn-612980. And
    while I found it quite ugly and im sure there are better ways to fix
    this... (namely we could fix Opcode​::_safe_call_sv to do something
    similar, or fix it as diagnosed in
    http​://rt.perl.org/rt3/Ticket/Display.html?id=60374#txn-491794)
    I was more or less looking for some kind of validation as to the
    approach :).

    I don't think the "detect if we are returning an anon sub" approach will
    work because the reval() may have compiled some named subs that will be
    invoked later. That's the case with PostgrSQL's embedded plperl, for example.

    http​://rt.perl.org/rt3/Public/Bug/Display.html?id=60374#txn-491794 says
      "pp_sort uses CopSTASH(PL_curcop) to get the package stash (symbol
      table) in order to locate the $a and $b variables in it."

    While pp_sort does call CopSTASH(PL_curcop) directly, all it does with
    it is store the value in PL_sortstash which, as far as I can tell, isn't
    used or documented anywhere at all. (It could possibly be deleted.)

    The code to lookup the $a and $b uses​:
      PL_firstgv = gv_fetchpvs("a", GV_ADD|GV_NOTQUAL, SVt_PV);
      PL_secondgv = gv_fetchpvs("b", GV_ADD|GV_NOTQUAL, SVt_PV);
    and it's Perl_gv_fetchpvn_flags that ends up calling CopSTASH(PL_curcop).

    If there was a way to get the stash pointer (not name) that was in
    effect at the time the code was compiled we could use that to lookup $a
    and $b.

    There are stash pointers, er, stashed in lots of places.
    One approach might be CvSTASH(find_runcv(...))

    Tim.

  19. p5pRT commented on Nov 9, 2009

    @p5pRT
    p5pRT
    on Nov 9, 2009
    Author

    From badalex@gmail.com

    On Sun, Nov 8, 2009 at 16​:36, Tim Bunce <Tim.Bunce@​pobox.com> wrote​:

    I don't think the "detect if we are returning an anon sub" approach will
    work because the reval() may have compiled some named subs that will be
    invoked later. That's the case with PostgrSQL's embedded plperl, for example.

    BTW I just tested and it does fix PostgreSQL

    create or replace function trustedsort()
    returns int
    as $$
    my @​arr = (5, 4, 3, 2, 1);
    my @​sorted = sort { elog(NOTICE, "$a $b"); $a <=> $b } @​arr;
    return 1;
    $$
    language 'plperl';

    unpatched Safe.pm​:
    select trustedsort();
    NOTICE​:
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​:
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​:
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​:
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​:
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​:
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​:
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​:
    CONTEXT​: PL/Perl function "trustedsort"

    trustedsort

      1
    (1 row)

    w patched​:
    select trustedsort();
    NOTICE​: 5 4
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​: 3 2
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​: 4 2
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​: 4 3
    CONTEXT​: PL/Perl function "trustedsort"
    NOTICE​: 2 1
    CONTEXT​: PL/Perl function "trustedsort"
    trustedsort

      1

  20. p5pRT commented on Nov 9, 2009

    @p5pRT
    p5pRT
    on Nov 9, 2009
    Author

    From @timbunce

    On Sun, Nov 08, 2009 at 05​:56​:21PM -0700, Alex Hunsaker wrote​:

    On Sun, Nov 8, 2009 at 16​:36, Tim Bunce <Tim.Bunce@​pobox.com> wrote​:

    I don't think the "detect if we are returning an anon sub" approach will
    work because the reval() may have compiled some named subs that will be
    invoked later. That's the case with PostgrSQL's embedded plperl, for example.

    BTW I just tested and it does fix PostgreSQL

    Ah, yes, PostgreSQL's embedded plperl does compile anon-subs. Thanks.
    Also your approach does seem to fix named subs invoked via the anon sub.
    It's unfortunate that the cost is two extra sub calls to invoke the
    anon sub :(

    I've attached a patch to Safe, based on your idea, slightly optimized
    and generalized, and including some tests.

    I've tested on a bunch of perl's I have lying around​:
    $ foreachperl makefreshtest
    [...]
      ok​: /usr/local/perl511-i/bin
      ok​: /usr/local/perl510-pure/bin
      ok​: /usr/local/perl510-thr64/bin
      ok​: /usr/local/perl510/bin
      ok​: /usr/local/perl58-i/bin
      ok​: /usr/local/perl588-64/bin
      ok​: /usr/local/perl588-thr/bin
      ok​: /usr/local/perl589-i/bin
      ok​: /usr/local/perl589/bin

    Thanks Alex!

    Tim.

  21. p5pRT commented on Nov 9, 2009

    @p5pRT
    p5pRT
    on Nov 9, 2009
    Author

    From @timbunce

    Safe-2.19-perl-rt-60374.patch 
    diff -rNu Safe-2.19-orig/MANIFEST Safe-2.19/MANIFEST
--- Safe-2.19-orig/MANIFEST	2009-06-28 15:14:49.000000000 +0100
+++ Safe-2.19/MANIFEST	2009-11-09 15:56:28.000000000 +0000
@@ -8,5 +8,6 @@
 t/safe3.t
 t/safeload.t
 t/safeops.t
+t/safesort.t
 t/safeuniversal.t
 META.yml                                 Module meta-data (added by MakeMaker)
diff -rNu Safe-2.19-orig/Safe.pm Safe-2.19/Safe.pm
--- Safe-2.19-orig/Safe.pm	2009-08-25 08:41:11.000000000 +0100
+++ Safe-2.19/Safe.pm	2009-11-09 16:10:47.000000000 +0000
@@ -2,6 +2,9 @@
 
 use 5.003_11;
 use strict;
+use Scalar::Util qw(reftype);
+use Config qw(%Config);
+use constant is_usethreads => $Config{usethreads};
 
 $Safe::VERSION = "2.19";
 
@@ -288,8 +291,26 @@
     my ($obj, $expr, $strict) = @_;
     my $root = $obj->{Root};
 
-    my $evalsub = lexless_anon_sub($root,$strict, $expr);
-    return Opcode::_safe_call_sv($root, $obj->{Mask}, $evalsub);
+    my $evalsub = lexless_anon_sub($root, $strict, $expr);
+    my @ret = (wantarray)
+        ?        Opcode::_safe_call_sv($root, $obj->{Mask}, $evalsub)
+        : scalar Opcode::_safe_call_sv($root, $obj->{Mask}, $evalsub);
+
+    # RT#60374: Safe.pm sort {} bug with -Dusethreads
+    # If the Safe eval returns a code ref in a perl compiled with usethreads
+    # then wrap code ref with _safe_call_sv so that, when called, the
+    # execution will happen with the compartment fully 'in effect'.
+    # Needed to fix sort blocks that reference $a & $b and
+    # possibly other subtle issues.
+    if (is_usethreads()) {
+        for my $ret (@ret) { # edit (via alias) any CODE refs
+            next unless (reftype($ret)||'') eq 'CODE';
+            my $sub = $ret; # avoid closure problems
+            $ret = sub { Opcode::_safe_call_sv($root, $obj->{Mask}, $sub) };
+        }
+    }
+
+    return (wantarray) ? @ret : $ret[0];
 }
 
 sub rdo {
diff -rNu Safe-2.19-orig/t/safesort.t Safe-2.19/t/safesort.t
--- Safe-2.19-orig/t/safesort.t	1970-01-01 01:00:00.000000000 +0100
+++ Safe-2.19/t/safesort.t	2009-11-09 15:53:10.000000000 +0000
@@ -0,0 +1,37 @@
+#!./perl -w
+$|=1;
+BEGIN {
+    if($ENV{PERL_CORE}) {
+	chdir 't' if -d 't';
+	@INC = '../lib';
+    } 
+    require Config; import Config;
+    if ($Config{'extensions'} !~ /\bOpcode\b/ && $Config{'osname'} ne 'VMS') {
+        print "1..0\n";
+        exit 0;
+    }
+}
+
+use Safe 1.00;
+use Test::More tests => 4;
+
+my $safe = Safe->new('PLPerl');
+$safe->permit_only(qw(:default sort));
+
+my $func = $safe->reval(<<'EOS');
+
+    # uses quotes in { "$a" <=> $b } to avoid the optimizer replacing the block
+    # with a hardwired comparison
+    { package Pkg; sub p_sort { return sort { "$a" <=> $b } qw(2 1 3); } }
+                   sub l_sort { return sort { "$a" <=> $b } qw(2 1 3); }
+
+    return sub { return join(",",l_sort()), join(",",Pkg::p_sort()) }
+
+EOS
+
+is $@, '', 'reval should not fail';
+is ref $func, 'CODE', 'reval should return a CODE ref';
+
+my ($l_sorted, $p_sorted) = $func->();
+is $l_sorted, "1,2,3";
+is $p_sorted, "1,2,3";

  22. p5pRT commented on Nov 9, 2009

    @p5pRT
    p5pRT
    on Nov 9, 2009
    Author

    From david@kineticode.com

    On Nov 9, 2009, at 8​:21 AM, Tim Bunce wrote​:

    Ah, yes, PostgreSQL's embedded plperl does compile anon-subs. Thanks.
    Also your approach does seem to fix named subs invoked via the anon
    sub.
    It's unfortunate that the cost is two extra sub calls to invoke the
    anon sub :(

    I've attached a patch to Safe, based on your idea, slightly optimized
    and generalized, and including some tests.

    Perhaps it got lost in the shuffle, and I'm certainly glad to see a
    Safe.pm issue addressed, but is there still an existing issue with
    sort and -Dusethreads?

    Best,

    David

  23. p5pRT commented on Nov 9, 2009

    @p5pRT
    p5pRT
    on Nov 9, 2009
    Author

    From @timbunce

    On Mon, Nov 09, 2009 at 09​:10​:18AM -0800, David E. Wheeler wrote​:

    On Nov 9, 2009, at 8​:21 AM, Tim Bunce wrote​:

    Ah, yes, PostgreSQL's embedded plperl does compile anon-subs. Thanks.
    Also your approach does seem to fix named subs invoked via the anon sub.
    It's unfortunate that the cost is two extra sub calls to invoke the
    anon sub :(

    I've attached a patch to Safe, based on your idea, slightly optimized
    and generalized, and including some tests.

    Perhaps it got lost in the shuffle,

    It reached the ticket ok​:
    http​://rt.perl.org/rt3/Ticket/Display.html?id=60374#txn-628757

    and I'm certainly glad to see a Safe.pm issue addressed, but is there
    still an existing issue with sort and -Dusethreads?

    In http​://rt.perl.org/rt3/Ticket/Display.html?id=60374#txn-627795
    Eric Brine says he can't reproduce them, so
      http​://rt.perl.org/rt3/Public/Bug/Display.html?id=37508
      http​://rt.perl.org/rt3/Public/Bug/Display.html?id=30333
    should be closed. (I don't know who gets to do that.)

    Tim.

  24. p5pRT commented on Nov 10, 2009

    @p5pRT
    p5pRT
    on Nov 10, 2009
    Author

    From @nwc10

    On Sun, Nov 08, 2009 at 11​:36​:46PM +0000, Tim Bunce wrote​:

    On Fri, Nov 06, 2009 at 02​:30​:47PM -0700, Alex Hunsaker wrote​:

    Um the last patch I posted should...
    http​://rt.perl.org/rt3/Ticket/Display.html?id=60374#txn-612980. And
    while I found it quite ugly and im sure there are better ways to fix
    this... (namely we could fix Opcode​::_safe_call_sv to do something
    similar, or fix it as diagnosed in
    http​://rt.perl.org/rt3/Ticket/Display.html?id=60374#txn-491794)
    I was more or less looking for some kind of validation as to the
    approach :).

    I don't think the "detect if we are returning an anon sub" approach will
    work because the reval() may have compiled some named subs that will be
    invoked later. That's the case with PostgrSQL's embedded plperl, for example.

    http​://rt.perl.org/rt3/Public/Bug/Display.html?id=60374#txn-491794 says
    "pp_sort uses CopSTASH(PL_curcop) to get the package stash (symbol
    table) in order to locate the $a and $b variables in it."

    While pp_sort does call CopSTASH(PL_curcop) directly, all it does with
    it is store the value in PL_sortstash which, as far as I can tell, isn't
    used or documented anywhere at all. (It could possibly be deleted.)

    The code to lookup the $a and $b uses​:
    PL_firstgv = gv_fetchpvs("a", GV_ADD|GV_NOTQUAL, SVt_PV);
    PL_secondgv = gv_fetchpvs("b", GV_ADD|GV_NOTQUAL, SVt_PV);
    and it's Perl_gv_fetchpvn_flags that ends up calling CopSTASH(PL_curcop).

    If there was a way to get the stash pointer (not name) that was in
    effect at the time the code was compiled we could use that to lookup $a
    and $b.

    The logical way to do this would be to indirect via an entry in the
    subroutine's pad, as the reference would be correctly translated by the
    ithreads clone code. This probably isn't *trivial* to implement, as a
    subroutine can contain code compiled in more than one package, but I think*
    it would be viable at compile time to loop up/store if not there yet, the
    stash pointer, under the stash's name.

    There are stash pointers, er, stashed in lots of places.
    One approach might be CvSTASH(find_runcv(...))

    I think that that doesn't cope with

    package foo;
    sub bar {
      # here package is foo
      package baz;
      # here package is baz
    }

    Nicholas Clark

    * I think. I don't know this stuff well.

  25. p5pRT commented on Nov 10, 2009

    @p5pRT
    p5pRT
    on Nov 10, 2009
    Author

    From @timbunce

    On Tue, Nov 10, 2009 at 02​:56​:36PM +0000, Nicholas Clark wrote​:

    http​://rt.perl.org/rt3/Public/Bug/Display.html?id=60374#txn-491794 says
    "pp_sort uses CopSTASH(PL_curcop) to get the package stash (symbol
    table) in order to locate the $a and $b variables in it."

    While pp_sort does call CopSTASH(PL_curcop) directly, all it does with
    it is store the value in PL_sortstash which, as far as I can tell, isn't
    used or documented anywhere at all. (It could possibly be deleted.)

    The code to lookup the $a and $b uses​:
    PL_firstgv = gv_fetchpvs("a", GV_ADD|GV_NOTQUAL, SVt_PV);
    PL_secondgv = gv_fetchpvs("b", GV_ADD|GV_NOTQUAL, SVt_PV);
    and it's Perl_gv_fetchpvn_flags that ends up calling CopSTASH(PL_curcop).

    If there was a way to get the stash pointer (not name) that was in
    effect at the time the code was compiled we could use that to lookup $a
    and $b.

    The logical way to do this would be to indirect via an entry in the
    subroutine's pad, as the reference would be correctly translated by the
    ithreads clone code. This probably isn't *trivial* to implement, as a
    subroutine can contain code compiled in more than one package, but I think*
    it would be viable at compile time to loop up/store if not there yet, the
    stash pointer, under the stash's name.

    There are stash pointers, er, stashed in lots of places.
    One approach might be CvSTASH(find_runcv(...))

    I think that that doesn't cope with

    package foo;
    sub bar {
    # here package is foo
    package baz;
    # here package is baz
    }

    I couldn't get it to work even without that (but trying to follow the
    inside-safe vs outside-safe and compile-time vs run-time intricacies
    made my rusty old head spin, so I could have done something silly).

    Alex's approach, of arranging to execute the closure 'inside' the
    compartment, seems like a good idea from a 'safety' point of view
    anyway. The fact it works around the bug is a bonus.

    On the other hand, the fact it costs two extra sub calls is unfortunate.
    I tried to look for a way to the PL_defstash switch into pp_entersub but
    couldn't see a viable approach.

    * I think. I don't know this stuff well.

    Me neither. It's been many years since I helped develop Opcode and Safe.
    (I recall refering to Safe as a "failed experiment" more than once.)

    Tim.

  26. p5pRT commented on Nov 16, 2009

    @p5pRT
    p5pRT
    on Nov 16, 2009
    Author

    From @timbunce

    I'm not sure who to ask to apply the patch to Safe.pm.
    I've created a ticket on rt.cpan.org to ensure it doesn't slip though the cracks from that side​:
    https://rt.cpan.org/Ticket/Display.html?id=51574

  27. p5pRT commented on Nov 30, 2009

    @p5pRT
    p5pRT
    on Nov 30, 2009
    Author

    From @rgs

    Now fixed with Safe 2.20.

  28. p5pRT commented on Nov 30, 2009

    @p5pRT
    p5pRT
    on Nov 30, 2009
    Author

    @rgs - Status changed from 'open' to 'resolved'

  29. p5pRT
    closed this as completedon Nov 30, 2009

  30. p5pRT commented on Feb 2, 2010

    @p5pRT
    p5pRT
    on Feb 2, 2010
    Author

    From @steve-m-hay

    demerphq wrote on 2009-10-08​:

    2009/10/8 Curtis Jewell <lists.perl.perl5-porters@​csjewell.fastmail.us>​:

    On Wed, 07 Oct 2009 10​:26 -0700, "David E. Wheeler"
    <david@​kineticode.com> wrote​:

    On Oct 7, 2009, at 10​:24 AM, Rafael Garcia-Suarez wrote​:

    Even if we manage someone to fix threading, this will slow down
    sort(). Threading also has a non-negligeable performance impact on
    memory and speed of every perl program.

    I think we should advice people in INSTALL to build with threading
    only if they really need it, and tell OS vendors to disable threading
    on their system perls.

    Snow Leopard ships with a threaded Perl. Hell, I think Leopard did.
    That cat might have left the bag.
    And Strawberry is threaded.  If I switched it to non-threaded now, I'd
    be stood up to a wall and shot by the Padre people (they require
    threading) among others.
    Im inclined to say that a non-threaded win32 build is pretty close to
    broken.

    Why do you say that? I always used to use a non-threaded perl on Win32, and only switched to threaded when I had to because mod_perl-2 needs it. Even now I still don't enable PERL_IMPLICIT_SYS because I have no need for the fork() emulation, and prefer Perl's malloc() (which can't be enabled with PERL_IMPLICIT_SYS) to the CRT's version.

  31. p5pRT
    added
    Severity Low
    on Oct 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions