Return the "hv with aux" PVHV bodies to the correct arena

Move the assignments to SvFLAGS(sv) after the check for SvOOK(), and use
arena_index for the array index to PL_body_roots. Without both fixes,
del_body() would always be called with &PL_body_roots[SVt_PVHV], which is
wrong if the body was allocated from &PL_body_roots[HVAUX_ARENA_ROOT_IX].

PVHV body handling was changed recently by commit 94ee6ed79dbca73d:
    Split the XPVHV body into two variants "normal" and "with aux"

    Default to the smaller body, and switch to the larger body if we need to
    allocate a C<struct xpvhv_aux> (eg need an iterator).

    This restores the previous small size optimisation for hashes used as
    objects.

as part of changing where memory for C<struct xpvhv_aux> is allocated.

The new implementation uses two sizes of "bodies" for PVHVs - the default
body is unchanged, but when a hash needs a C<struct xpvhv_aux>, it now
swaps out the default body for a larger body with space for the struct.

(Previously it would reallocate the memory used for HvARRAY() to make space
for the struct there - an approach which requires more tracking of changed
pointers.)

That commit was subtly buggy in that on hash deallocation it didn't *return*
hash bodies to the correct arena. As-was, it would return both sizes of hash
body to the arena for the default (smaller) body. This was due to a
combination of two subtle bugs

1) the explicit reset of all SvFLAGS to SVf_BREAK would implicitly clear
   SVf_OOK. That flag bit set is needed to signal the different body size
2) The code must call del_body() with &PL_body_roots[arena_index], not
   &PL_body_roots[type]
   type is SVt_PVHV for both body sizes. arena_index is SVt_PVHV for the
   smaller (default) bodies, and HVAUX_ARENA_ROOT_IX for the larger bodies.

There were no memory errors or leaks (that would have been detectable by
tools such as ASAN or valgrind), but the bug meant that larger hash bodies
would never get re-used. So for code that was steady-state creating and
destroying hashes with iterators (or similar), instead of those bodies being
recycled via their correct arena, the interpreter would need to continuously
allocate new memory for that arena, whilst accumulating ever more unused
"smaller" bodies in the other arena.

I didn't spot this bug whilst reviewing commit 8461dd8ad90b2bce:
    don't overwrite the faked up type details for hv-with-aux

    CID 340472

which is in the same area of the code, but fixes a related problem.

Curiously when reporting CID 340738:

    Assigning value "SVt_IV" to "arena_index" here, but that stored value is
    overwritten before it can be used

Coverity also didn't spot that the code in question was unconditionally
unreachable, and warn about that, or modify its warning to note that.

(For the code prior to this commit SvOOK(sv) at that point could only ever
be false. This is obscured thanks to macros, but all these macros are
unconditionally defined to the same values, so static analysis should be
able to infer that they are actually constant, and reason accordingly.
To be fair, this is *hard*. But not impossible. Hence I infer that static
analysis tools still have room for improvement, and can make "mistakes".)

Ironically, defining PURIFY would hide this bug, because that disables the
use of arenas, instead allocating all bodies explicitly with malloc() and
releasing them with free(), and free() doesn't need to know the size of the
memory block, hence it wouldn't matter that the code had that size wrong.

1 files changed, 22 insertions, 18 deletions

diff --git a/sv.c b/sv.c
index 4a8c6d9c1f..aa8b97c315 100644
--- a/sv.c
+++ b/sv.c
@@ -6777,8 +6777,6 @@ Perl_sv_clear(pTHX_ SV *const orig_sv)
 
     while (sv) {
         U32 type = SvTYPE(sv);
-        U32 arena_index;
-        const struct body_details *sv_type_details;
         HV *stash;
 
         assert(SvREFCNT(sv) == 0);
@@ -7063,23 +7061,29 @@ Perl_sv_clear(pTHX_ SV *const orig_sv)
 
       free_body:
 
-        SvFLAGS(sv) &= SVf_BREAK;
-        SvFLAGS(sv) |= SVTYPEMASK;
+        {
+            U32 arena_index;
+            const struct body_details *sv_type_details;
 
-        if (type == SVt_PVHV && SvOOK(sv)) {
-            arena_index = HVAUX_ARENA_ROOT_IX;
-            sv_type_details = &fake_hv_with_aux;
-        }
-        else {
-            arena_index = type;
-            sv_type_details = bodies_by_type + arena_index;
-        }
-        if (sv_type_details->arena) {
-            del_body(((char *)SvANY(sv) + sv_type_details->offset),
-                     &PL_body_roots[type]);
-        }
-        else if (sv_type_details->body_size) {
-            safefree(SvANY(sv));
+            if (type == SVt_PVHV && SvOOK(sv)) {
+                arena_index = HVAUX_ARENA_ROOT_IX;
+                sv_type_details = &fake_hv_with_aux;
+            }
+            else {
+                arena_index = type;
+                sv_type_details = bodies_by_type + arena_index;
+            }
+
+            SvFLAGS(sv) &= SVf_BREAK;
+            SvFLAGS(sv) |= SVTYPEMASK;
+
+            if (sv_type_details->arena) {
+                del_body(((char *)SvANY(sv) + sv_type_details->offset),
+                         &PL_body_roots[arena_index]);
+            }
+            else if (sv_type_details->body_size) {
+                safefree(SvANY(sv));
+            }
         }
 
       free_head: