Fix out-of-bounds panic when a meta content value ends in a bare charset token#762
Fix out-of-bounds panic when a meta content value ends in a bare charset token#762noahskelton wants to merge 1 commit into
Conversation
…set token extract_a_character_encoding_from_a_meta_element indexed one byte past the end of the input when the content value ended in "charset" with optional trailing whitespace and no "=" after it (e.g. content="text/html; charset"). Guard the step-4 lookahead with .get() and return None when the input ends there.
|
Servo does not accept AI generated contributions: https://book.servo.org/contributing/getting-started.html#ai-contributions |
|
@simonwuelker thankyou for fixing the bug in another pull request, i'm very aware that open source maintainers are overburdened with AI generated pull requests, but I did make every effort to be honest about AI usage here and included a very clear repro and fix. I don't think you should punish honest disclosures with such a dismissal. The irony here is that AI was able to find this bug in the first place. I think at the very least an acknowledgement of submitting a patch and a bug is warranted (of course you have complete freedom to do what you want here, I just want to express my thoughts clearly). Regardless, thanks for fixing the issue. |
Our policy is not just caused by maintainer burden but also by ethical and environmental concerns: https://book.servo.org/contributing/getting-started.html#ai-contributions.
My comment was only related to this PR, not the issue. I don't deny that this was a real bug (whether it was found by AI or not) and I'm glad you took the time to report it. |
|
@simonwuelker That's fair - to be clear I just think a small acknowledgement in your original comment would have been the nice thing to do, I disagree with the AI policy but also can respect and understand it, but I think a more nuanced approach is beneficial for everyone (and would encourage other contributors). Thanks! |
AI Disclosure
This fix was made with the help of Claude Fable 5 and reviewed additionally with Codex GPT 5.5. The bug was encountered in the wild (production HTML scraping pipeline feeding pages through a downstream binding of this crate), and the fix and regression tests have been reviewed and verified by hand.
Summary
extract_a_character_encoding_from_a_meta_elementinhtml5ever/src/encoding.rspanics with an out-of-bounds index when a<meta http-equiv="Content-Type">element'scontentvalue ends in a barecharsettoken with no=after it, e.g.content="text/html; charset". Because the function runs on the parser's hot path (the in-head<meta>rule intree_builder/rules.rscalls it for everyhttp-equiv="Content-Type"element), any such page hard-panicsparse_document. In embedded/FFI contexts this surfaces as an uncatchable process abort:Pages like this exist in the wild — typically a Content-Type meta truncated mid-attribute (
...; charset), or pages that literally emitcontent="text/html; charset".Root cause
After matching the word
charset, step 3 of the WHATWG "extract a character encoding from a meta element" algorithm skips trailing ASCII whitespace, which can leaveposition == input.len()when nothing follows the token. Step 4 then reads the next byte with an unchecked index:(18 is the length of
text/html; charset.)Reproducer
Minimal offending page:
Any parse of it reaches the panic, e.g. with this temporary example (not included in the PR):
Before this change it aborts at
encoding.rs:39with the backtrace pointing atextract_a_character_encoding_from_a_meta_element; after it, it printsparsed ok.The fix
Guard the step-4 lookahead with
.get(). Per the spec, if there is no next character the outer loop can never find anothercharsetmatch, so the function returnsNone(no encoding):This is the only unchecked index in the function — everything after the
=already uses.get(position)?or range slices, which are safe atposition == input.len().Regression test
meta_element_charset_at_end_does_not_paniccovers the three wayspositioncan land at the end of the input:"text/html; charset","charset", and"charset \t"(trailing whitespace). All three panic before the fix and returnNoneafter it.cargo test -p html5ever,cargo fmtandcargo clippy -p html5everall pass.