libwww-perl / libwww-perl/HTML-Parser
decode_entities: mutation guard misses same-length in-place changes during entity-value fetch
Nobody has claimed this yet.
- Dominant language
- Perl
- Stars
- 7
- Forks
- 15
- Avg merge
- 12h 8m
- Merged PRs (30d)
- 2
Description
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
- 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. - Remove the hazard entirely (the final bullet on #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
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in util.c at decode_entities() and read the existing mutation guard, including the orig snapshot and the s..end unread region. Reproduce the ChangeInPlaceOnFetch case from the issue, then compare the guard-only and private-buffer options. Done means the in-place mutation test raises the documented error without regressing the existing memory-safety behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- perl
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100