Potential unsoundness in `referencing`

[Manishearth]

I was unsafe-reviewing the referencing crate and noticed this:

jsonschema/crates/jsonschema-referencing/src/anchors/mod.rs

Lines 29 to 36 in a1ffac5

	// SAFETY: The pointer is valid as long as the registry exists
	unsafe {
	std::str::from_utf8_unchecked(std::slice::from_raw_parts(
	self.ptr.load(Ordering::Relaxed),
	self.len,
	))
	}
	}

It's very unclear if this invariant is upheld properly: anchors get threaded through a lot of the code and there are no comments anywhere talking about this

Furthermore, self-referential code is notorious in Rust for being hard to get right: even when the conceptual lifetimes are right there are many subtle things around Rust's aliasing model that you can get wrong.

Some alternate routes:

• Just clone the string
• Use an existing self-referential crate
• See if you can make it work with lifetimes. I'm not convinced this is possible.
• Split out the Anchors map into an AnchorContext type that is not stored within the Registry but instead passed to it, allowing it to reference the registry.

[Stranger6667]

Hi @Manishearth

Thank you for opening this! Indeed, the current implementation is somewhat sketchy and very poorly documented. My thinking is:

• Pin<Arc<ValueWrapper>> keeps the addresses of documents stable & alive as long as that Arc exist
• Anchor is internal only, so the users can't extract one and outlive the registry
• Registry is not mutated after creation

I think it should be sound, but surely I may be wrong about it (I am inclined to think I have missed something there). There were quite a lot of failures with Miri when I implemented it originally, but I've resolved them (also, I think the fuzzing builds found something too!).

All of these dances with pin & pointers were due to Validator owning a Registry because of the compiler implementation detail (it was compiling & caching recursive refs during validation). As of the release this morning (v0.34.0), this hack in Validator was removed (and recursive refs are handled differently fully at the validator compile-time).

For this reason, I am planning to migrate to just storing references in Registry as owned documents are not needed anymore (it will also improve performance), + maybe rework the interfaces inside referencing so it is a bit more convenient to use.

Thank you once again for bringing it up - I'll prioritize the Registry rework.

EDIT: I've realized that registries also fetch external documents, so some self-referential helpers will probably be needed. I'll take a look