servo / servo/string-cache

Make string-cache compatible with Strict Provenance

Open
#260 6 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
212
Forks
84
Avg merge
1d 2h
Merged PRs (30d)
6

Description

Original title: Status of provenance/aliasing issues detected by Miri

Hi, I wish I were being helpful here but I'm just reporting the status of some hackery I've done. (hours wasted here, etc)

This is one of the most-downloaded crates that roundtrips pointer values through integers. I'm looking into most such crates to understand if it's possible to rewrite such roundtrips or if there's something better that can be done to support what libraries want to do.

For this crate, I do not think that it is possible to patch out the roundtrips, but the problem isn't the round-tripping. This crate wants to store pointers inside Atom, and to do that in a well-defined way we need a type which is capable of storing provenance and also plain old bytes, so we could use a raw pointer type or MaybeUninit. But we want to uphold the niche which it currently has, so the only type which is eligible is NonNull. But putting NonNull in that field breaks string-cache-codegen, because it puts Atom in a macro_rules! pattern:

error: to use a constant of type `std::ptr::NonNull` in a pattern, `std::ptr::NonNull` must be annotated with `#[derive(PartialEq, Eq)]`
    --> /tmp/markup5ever-0.11.0/target/miri/x86_64-unknown-linux-gnu/debug/build/markup5ever-85445da718f099e3/out/generated.rs:3381:39
     |
3381 | ...html") => { $ crate :: ATOM_NAMESPACE__68_74_74_70_3A_2F_2F_77_77_77_2E_77_33_2E_6F_72_67_2F_31_39_39_39_2F_78_68_74_6D_6C } ;
     |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
    ::: interface/tree_builder.rs:89:9
     |
89   |         expanded_name!(html "template") => flags.template = true,
     |         ------------------------------- in this macro invocation
     |
     = note: this error originates in the macro `namespace_url` (in Nightly builds, run with -Z macro-backtrace for more info)

I've put up the changes that would remove the ptr-int-ptr roundtrips (as well as some aliasing issues with Box that current SB has) in a branch in case anyone is interested: https://github.com/servo/string-cache/compare/master...saethlin:fix-aliasing

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 by reviewing the linked fix-aliasing branch and the Strict Provenance report, then inspect Atom usage in string-cache-codegen and the generated pattern shown at generated.rs:3381, reached from interface/tree_builder.rs:89. Done means preserving Atom's niche while avoiding the pointer roundtrips and making the macro_rules! pattern compile under the reported provenance constraints.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.