From 17535c984ae7d68304cc6e7b997240683f7fcadf Mon Sep 17 00:00:00 2001
From: David Mitchell <davem@iabyn.com>
Date: Thu, 18 Apr 2024 21:33:08 +0100
Subject: [PATCH] fix refcount on cloned constant state subs

Perl was doing this:

    $ perl -e'sub { CORE::state sub FOO () { 42 } }'
    Attempt to free unreferenced scalar: ...
    $

This warning was in particular now showing up on stderr on bleed builds:
ever since the recent addition of similar code to Deparse.t with
v5.39.9-33-g4a55343c55.

When a sub is made constant, it is converted into an XS sub, and the
IV(42) SV is stored in the CV's CvXSUBANY(cv).any_sv field.

But state subs (even const ones) get cloned if wrapped within an outer
anon sub and then that outer sub gets cloned. And it turns out that when
a const state sub is cloned, the ref count of that const SV wasn't being
incremented.

The fix is trivial. But there were two possible ways to fix it. The
approach I chose was to fix the cloning code so that it increments on
CvCONST(cv) being true in addition to on CvREFCOUNTED_ANYSV(cv) being
true.

The other approach (and arguably more logically correct) would be to set
the CVf_REFCOUNTED_ANYSV flag on const subs too, but this involves
modifying the code in multiple places, e.g.  newMYSUB(), newATTRSUB_x(),
newCONSTSUB_flags(), and makes it more likely that CPAN XS code out
there which cargo-cults similar code would also need fixing. So my fix
is simpler, more robust, but less satisfying.

Note that before 5.36.0, the failing code example above would segfault
rather than warn.
---
 pad.c        |  2 +-
 t/op/state.t | 17 ++++++++++++++++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/pad.c b/pad.c
index d4a20ef84cbb..921c10d1cefa 100644
--- a/pad.c
+++ b/pad.c
@@ -2235,7 +2235,7 @@ S_cv_clone(pTHX_ CV *proto, CV *cv, CV *outside, HV *cloned)
     if (UNLIKELY(CvISXSUB(proto))) {
         CvXSUB(cv)    = CvXSUB(proto);
         CvXSUBANY(cv) = CvXSUBANY(proto);
-        if (CvREFCOUNTED_ANYSV(cv))
+        if (CvREFCOUNTED_ANYSV(cv) || CvCONST(cv))
             SvREFCNT_inc(CvXSUBANY(cv).any_sv);
     }
     else {
diff --git a/t/op/state.t b/t/op/state.t
index 747e316f226a..f309b4ec3fca 100644
--- a/t/op/state.t
+++ b/t/op/state.t
@@ -9,7 +9,7 @@ BEGIN {
 
 use strict;
 
-plan tests => 164;
+plan tests => 166;
 
 # Before loading feature.pm, test it with CORE::
 ok eval 'CORE::state $x = 1;', 'CORE::state outside of feature.pm scope';
@@ -503,6 +503,21 @@ for (1,2) {
     is($s, "-1-", "state with multiconcat pass $_");
 }
 
+# This caused 'Attempt to free unreferenced scalar' because the SV holding
+# the value of the const state sub wasn't having its ref count incremented
+# when the sub was cloned.
+
+{
+    my @warnings;
+    local $SIG{__WARN__} = sub { push @warnings, @_ };
+    my $e = eval 'my $s = sub { state sub FOO () { 42 } }; 1;';
+    is($e, 1, "const state sub ran ok");
+    ok(!@warnings, "no 'Attempt to free unreferenced scalar'")
+        or diag "got these warnings:\n@warnings";
+}
+
+
+
 __DATA__
 (state $a) = 1;
 (state @a) = 1;