decode_entities: mutation guard misses same-length in-place changes during entity-value fetch #70

While reviewing #69 we found that the mutation guard it adds is a guard, not a
cure — and it has one blind spot worth tracking separately.

Background

decode_entities() (util.c) resolves entity values with hv_fetch/SvPV,
which can run arbitrary Perl — a tied entity2char, or an overloaded value.
The guard added in #69 snapshots SvPVX/SvCUR/SvLEN/SvUTF8 before the
lookup and, afterwards, dies with "String being decoded was modified while fetching an entity value" if any of the four changed. That correctly closes
the memory-safety hazard (free / realloc / undef / shrink), which was the goal
of #69.

The gap

A same-length, in-place write during the fetch leaves all four compared
values unchanged, so the guard does not fire. The decoder then keeps reading
from a buffer whose not-yet-consumed bytes were rewritten underneath it,
silently producing altered output.

This is not a memory-safety problem — the write is in-bounds and the length
is unchanged — but it is a silent integrity gap, and it contradicts the
documented contract in HTML::Entities ("If that code modifies the string
being decoded, this routine dies").

Reproduction

A tied table whose FETCH rewrites a not-yet-read byte in place:

use HTML::Entities qw(_decode_entities);
{ package M; require Tie::Hash; our @ISA = ("Tie::StdHash"); our $t;
  sub FETCH { substr($$t, 50, 1, "X"); "y" } }   # offset 50 is still unread
my %h; tie %h, "M";
my $s = "&foo;" . ("a" x 100);
$M::t = \$s;
_decode_entities($s, \%h);
# no exception is thrown; the injected "X" appears in the decoded output

The failing test that documents this (removed from #69 so its CI stays green):

{
    package ChangeInPlaceOnFetch;
    require Tie::Hash;
    our @ISA = ("Tie::StdHash");
    our $target;
    sub FETCH { substr($$target, 1, 1, "X"); "x" }
}
{
    my %tied;
    tie %tied, "ChangeInPlaceOnFetch";
    my $s = "&foo;" . ("a" x 100);
    $ChangeInPlaceOnFetch::target = \$s;
    eval { _decode_entities($s, \%tied) };
    like(
        $@,
        qr/modified while fetching an entity value/,
        "_decode_entities() detects an in-place mutation while fetching"
    );
}

Options

1. Extend the guard to detect in-place changes too — e.g. after each fetch,
   compare the still-unread region (s..end) against the original-string
   snapshot the code already keeps (orig), and die on a mismatch. Cheap, and
   makes the test above pass.
2. Remove the hazard entirely (the final bullet on GH-68 Fix self-referential entity expansion #69): decode into a
   private buffer and assign the result at the end, which also removes
   grow_gap. This is a rewrite of a hot loop and is the more thorough fix.

Filed as its own ticket per the discussion on #69; the guard-only approach
there is intentional and sufficient for the memory-safety fix.

🤖 Generated with Claude Code · Opus 4.8