Skip to content

Avoid panic in parse_legacy_color on whitespace-only input - #1

Closed
nicoburns wants to merge 1 commit into
mainfrom
devin/1785182052-fix-legacy-color-panic
Closed

Avoid panic in parse_legacy_color on whitespace-only input#1
nicoburns wants to merge 1 commit into
mainfrom
devin/1785182052-fix-legacy-color-panic

Conversation

@nicoburns

@nicoburns nicoburns commented Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

parse_legacy_color only checked emptiness before stripping HTML whitespace, so a value like <body bgcolor=" "> reached step 9's input.as_bytes()[0] with an empty string and panicked with an index-out-of-bounds, a crash triggerable from untrusted HTML.

Two changes made in style/servo/attr.rs:

  • Check for empty input again after stripping whitespace:
  • Use the built-in stdlib strip_prefixes function which is tolerant of empty input:

@nicoburns nicoburns self-assigned this Jul 27, 2026
@staging-devin-ai-integration

staging-devin-ai-integration Bot commented Jul 27, 2026

Copy link
Copy Markdown

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR that start with 'DevinAI' or '@devin'.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@staging-devin-ai-integration

staging-devin-ai-integration Bot commented Jul 27, 2026

Copy link
Copy Markdown

Runtime verification — panic fix confirmed

Verified with a throwaway integration test (not committed) run as cargo test -p stylo --features servo, plus a before/after comparison where the two hunks were temporarily reverted in the working tree.

🟢 On this branch: whitespace-only input returns Err, no panic (3/3 tests pass)

Post-fix: all tests pass

  • parse_legacy_color on " ", "\t", "\n", "\r", "\u{c}", " \t\n", "" → Err(()), no panic
  • End-to-end through the real caller AttrValue::from_legacy_color (the <body bgcolor=" "> path) → Color(s, None) for every whitespace-only value, and Color("#ff0000", Some(rgb(255,0,0))) for a normal value
  • Non-ASCII "§§§" → Ok(black), no panic (confirms strip_prefix avoids mid-codepoint slicing)
🔴 With the two hunks reverted, the same inputs panic (bug reproduced)

Pre-fix: index out of bounds panic

thread 't1_whitespace_only_returns_err_without_panic' panicked at style/servo/attr.rs:574:8:
index out of bounds: the len is 0 but the index is 0
test result: FAILED. 1 passed; 2 failed
Regression: normal parsing unchanged
  • "red", "#f00", "#ff0000", " #ff0000 ", "ff0000" → srgb_legacy(255,0,0,1.0); "#0000ff" → srgb_legacy(0,0,255,1.0); "transparent" → Err(())
  • "#" and " # " → Ok(black) (spec behavior: trims to "#", step 11 pads to "000") — unchanged from before
  • 128-char truncation still exact: "0"*126+"f" (127 chars) → rgb(0,0,240); 128 chars → rgb(0,0,255); 129 and 200 chars → rgb(0,0,255)
⚠️ Pre-existing, unrelated: crate unit tests don't compile under --features servo

cargo test -p stylo --features servo fails to build the lib test target — style/selector_parser.rs tests reference gecko-only PseudoElement::MozRubyText / FirstLine (E0599). Reproduced identically on main, so not caused by this PR.

@nicoburns nicoburns changed the title Avoid panic in parse_legacy_color on whitespace-only input Avoid panic in parse_legacy_color on whitespace-only input Jul 27, 2026
@nicoburns nicoburns closed this Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nicoburns nicoburns

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@nicoburns