libwww-perl / libwww-perl/HTML-Parser

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

Open
#70 0 comments 0 reactions 0 assignees View on GitHub

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

  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 #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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.