av.c - av_store() do the refcount dance around magic av's

The api for av_store() says it is the callers responsibility to call
SvREFCNT_inc() on the stored item after the store is successful.

However inside of av_store() we store the item in the C level array before we
trigger magic. To a certain extent this is required because mg_set(av) needs
to be able to see the newly stored item.

But if the mg_set() or other magic associated with the av_store() operation
fails, we would end up with a double free situation, as we will long jump up
the stack above and out of the call to av_store(), freeing the mortal as we go
(via Perl_croak()), but leaving the reference to the now freed pointer in the
array. When the next SV is allocated the reference will be reused, and then we
are in a double free scenario.

I see comments in pp_aassign talking about defusing the temps stack for the
parameters it is passing in, and things like this, which at first looked
related. But that commentary doesn't seem that relevant to me, as this bug
could happen any time a scalar owned by one data structure was copied into an
array with set magic which could die. Eg, I can easily imagine XS code that
expected code like this (assume it handles magic properly) to work:

    SV **svp = av_fetch(av1,0,1);
    if (av_store(av2,0,*svp))
        SvREFCNT_inc(*svp);

but if av2 has set magic and it dies the end result will be that both av1 and
av2 contain a visible reference to *svp, but its refcount will be 1. So I think
this is a bug regardless of what pp_aassign does.

This fixes https://github.com/Perl/perl5/issues/20675

1 files changed, 41 insertions, 0 deletions

diff --git a/av.c b/av.c
index ae902dd3ec..d9868f4407 100644
--- a/av.c
+++ b/av.c
@@ -373,10 +373,47 @@ Perl_av_store(pTHX_ AV *av, SSize_t key, SV *val)
     }
     else if (AvREAL(av))
         SvREFCNT_dec(ary[key]);
+
+    /* store the val into the AV before we call magic so that the magic can
+     * "see" the new value. Especially set magic on the AV itself. */
     ary[key] = val;
+
     if (SvSMAGICAL(av)) {
         const MAGIC *mg = SvMAGIC(av);
         bool set = TRUE;
+        /* We have to increment the refcount on val before we call any magic,
+         * as it is now stored in the AV (just before this block), we will
+         * then call the magic handlers which might die/Perl_croak, and
+         * longjmp up the stack to the most recent exception trap. Which means
+         * the caller code that would be expected to handle the refcount
+         * increment likely would never be executed, leading to a double free.
+         * This can happen in a case like
+         *
+         * @ary = (1);
+         *
+         * or this:
+         *
+         * if (av_store(av,n,sv)) SvREFCNT_inc(sv);
+         *
+         * where @ary/av has set magic applied to it which can die. In the
+         * first case the sv representing 1 would be mortalized, so when the
+         * set magic threw an exception it would be freed as part of the
+         * normal stack unwind. However this leaves the av structure still
+         * holding a valid visible pointer to the now freed value. In practice
+         * the next SV created will reuse the same reference, but without the
+         * refcount to account for the previous ownership and we end up with
+         * warnings about a totally different variable being double freed in
+         * the form of "attempt to free unreferenced variable"
+         * warnings/errors.
+         *
+         * https://github.com/Perl/perl5/issues/20675
+         *
+         * Arguably the API for av_store is broken in the face of magic. Instead
+         * av_store should be responsible for the refcount increment, and only
+         * not do it when specifically told to do so (eg, when storing an
+         * otherwise unreferenced scalar into an AV).
+         */
+        SvREFCNT_inc(val);  /* see comment above */
         for (; mg; mg = mg->mg_moremagic) {
           if (!isUPPER(mg->mg_type)) continue;
           if (val) {
@@ -389,6 +426,10 @@ Perl_av_store(pTHX_ AV *av, SSize_t key, SV *val)
         }
         if (set)
            mg_set(MUTABLE_SV(av));
+        /* And now we are done the magic, we have to decrement it back as the av_store() api
+         * says the caller is responsible for the refcount increment, assuming
+         * av_store returns true. */
+        SvREFCNT_dec(val);
     }
     return &ary[key];
 }