Branch: refs/heads/blead
Home: https://github.com/Perl/perl5
Commit: 37d796a5a62dfb0753d9d8f0fc91886e05965c8b
https://github.com/Perl/perl5/commit/37d796a5a62dfb0753d9d8f0fc91886e05965c8b
Author: Nicholas Clark <n...@ccl4.org>
Date: 2021-12-18 (Sat, 18 Dec 2021)
Changed paths:
M hv.c
Log Message:
-----------
Document the HV aux struct free/release logic in hv_undef_flags()
struct xpvhv_aux holds 4 pointers which can point to memory which needs
explicit release - document how hv_undef_flags() is directly responsible
for freeing xhv_mro_meta, indirectly responsible for freeing xhv_eiter,
and by implication can't free xhv_backreference or xhv_name_u.
Commit: d9e6229a98412e8a44e463190ef148867e3e000a
https://github.com/Perl/perl5/commit/d9e6229a98412e8a44e463190ef148867e3e000a
Author: Nicholas Clark <n...@ccl4.org>
Date: 2021-12-18 (Sat, 18 Dec 2021)
Changed paths:
M hv.c
Log Message:
-----------
Don't clear the flag bit SVf_OOK in hv_undef_flags()
Now that HvAUX(hv) is allocated as part of the HV's body, it's imperative
to retain the flag that indicates which size of body has been allocated.
Otherwise, if we clear the flag, sv_clear() believes that the body is the
smaller of the two possible sizes, and so returns it to the correct arena
for that body size. This doesn't result in out-of-bounds memory reads, or
even memory leaks (when full cleanup runs with PERL_DESTRUCT_LEVEL=2),
because all bodies are still correctly tracked. However, without this bug
fixed, code that takes the same hash and repeatedly
1) assigns entries
2) iterates the hash
3) undef %hash
will request one new (larger) hash body for each iteration of the loop,
but each time return it to the arena for the smaller hash bodies, with the
result that the interpreter will steadily request ever more memory for
arenas for larger hash bodies, until memory is exhausted.
Commit: 7ab3ff6e3556756a0cabd4d4079485903f1aaabe
https://github.com/Perl/perl5/commit/7ab3ff6e3556756a0cabd4d4079485903f1aaabe
Author: Nicholas Clark <n...@ccl4.org>
Date: 2021-12-18 (Sat, 18 Dec 2021)
Changed paths:
M sv.c
Log Message:
-----------
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.
Commit: 5d73aca10c2e46b1f9ea1687f0f3e1a95a489653
https://github.com/Perl/perl5/commit/5d73aca10c2e46b1f9ea1687f0f3e1a95a489653
Author: Nicholas Clark <n...@ccl4.org>
Date: 2021-12-18 (Sat, 18 Dec 2021)
Changed paths:
M hv.c
Log Message:
-----------
No need to Renew() HvARRAY() to the same size in S_hv_auxinit()
Fix a thinko/missed simplification that should have been part of
commit 53a41f9c2c0671d8:
Inline the xhv_aux struct in the main hash body
For now, memory for this structure is always allocated, even though it
isn't always flagged as being present.
Prior to that commit the Renew() in S_hv_auxinit() had been needed because
the memory that HvARRAY() points to needed to be expanded from "just the
array of HE pointers" to "that, plus space for the xhv_aux struct.
With that commit, the xhv_aux struct was no longer stored in the same
memory block, so there was no need to reallocate storage. Hence the
Renew() should be completely removed. What that commit actually did was
neatly change the parameters to the Renew() call such that it reallocated
storage to the exact same size, which isn't wrong, but is makework. Oops.
Compare: https://github.com/Perl/perl5/compare/3d8a032cdae9...5d73aca10c2e
