Skip to content

Commit

Permalink
pp_aassign: remove panic check under PERL_RC_STACK
Browse files Browse the repository at this point in the history
There is some code in S_aassign_copy_common() (helper function for
pp_aassign()) which checks for SvIS_FREED() of the current arg.
The comments imply that the check is there specifically to help a TODO
test in t/op/sort.t not randomly crash. That test does stuff which
triggers a premature free of args on the stack.

This test was later disabled entirely, as whatever panics or crashes
were being triggered weren't being caught by the eval {} it was wrapped
in.

This commit, only on PERL_RC_STACK builds, skips the SvIS_FREED check,
and re-enables the test in sort.t (as a real test, not a TODO).
  • Loading branch information
iabyn committed Nov 16, 2023
1 parent d899222 commit 33d9495
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 13 deletions.
7 changes: 7 additions & 0 deletions pp_hot.c
Expand Up @@ -2493,13 +2493,20 @@ S_aassign_copy_common(pTHX_ SV **firstlelem, SV **lastlelem,

TAINT_NOT; /* Each item is independent */

#ifndef PERL_RC_STACK
/* The TODO test was eventually commented out. It's now been
* revived, but only on PERL_RC_STACK builds. Continue
* this hacky workaround otherwise - DAPM Sept 2023 */

/* Dear TODO test in t/op/sort.t, I love you.
(It's relying on a panic, not a "semi-panic" from newSVsv()
and then an assertion failure below.) */
if (UNLIKELY(SvIS_FREED(svr))) {
Perl_croak(aTHX_ "panic: attempt to copy freed scalar %p",
(void*)svr);
}
#endif

/* avoid break flag while copying; otherwise COW etc
* disabled... */
SvFLAGS(svr) &= ~SVf_BREAK;
Expand Down
22 changes: 9 additions & 13 deletions t/op/sort.t
Expand Up @@ -7,7 +7,7 @@ BEGIN {
set_up_inc('../lib');
}
use warnings;
plan(tests => 204);
plan(tests => 205);
use Tie::Array; # we need to test sorting tied arrays

# these shouldn't hang
Expand Down Expand Up @@ -884,18 +884,14 @@ cmp_ok($answer,'eq','good','sort subr called from other package');
}
}



# I commented out this TODO test because messing with FREEd scalars on the
# stack can have all sorts of strange side-effects, not made safe by eval
# - DAPM.
#
#{
# local $TODO = "sort should make sure elements are not freed in the sort block";
# eval { @nomodify_x=(1..8);
# our @copy = sort { undef @nomodify_x; 1 } (@nomodify_x, 3); };
# is($@, "");
#}
SKIP:
{
skip "freed args not under PERL_RC_STACK", 1
unless (Internals::stack_refcounted() & 1);
eval { @nomodify_x=(1..8);
our @copy = sort { undef @nomodify_x; 1 } (@nomodify_x, 3); };
is($@, "");
}


# Sorting shouldn't increase the refcount of a sub
Expand Down

0 comments on commit 33d9495

Please sign in to comment.