sanitize v3 status / next steps

[cmyr]

Not sure an issue is the best place for this, but whatever. For background, see sneaking up on sanitize, which describes a strategy for landing the sanitize pattern in a fully safe way, with any potentially unsafe work being done separately in the future.

My focus for much of this work has been on figuring out how to write it in such a way that we can reasonably land it; this means breaking the work up into manageable stages. In order to actually verify that this approach makes sense, though, I have also gone and put together a couple 'assembled' branches that include a everything we need to use sanitize on real tables, for the purposes of benchmarking.

For benchmarking, I have been using the simple shape benchmark target in harfrust. I've benchmarked two different versions of this work: the simple safe version, which we should be able to land without controversy, as well as an unsafe version, that shows the possible gains if we are able to eventually move to unchecked reads. The unsafe version is maximalist in terms of what is unsafe; in theory there could also be a middle ground, where unsafe is only used for the set of fields that are guaranteed to always be present.

Summary (% change vs baseline)

to run benchmarks yourselves, use the following harfrust branches:

• baseline -- bench-sanitize-v3-baseline -- stock FontRead (minimal validation, but runs on each read)
• sanitize -- bench-sanitize-v3 -- safe FastRead (validate once, skip re-validation on subsequent access)
• unsafe -- bench-sanitize-v3-unsafe -- unsafe fast path (unchecked access after initial validation)

the results below are just me running cargo bench --bench=shaping on each target ten times, and averaging the results. This is pretty crude, but should give us averages we are reasonably confident in.

Benchmark	baseline (ms)	sanitize (ms)		unsafe (ms)
Amiri / fa-thelittleprince	30.23	28.26	-6.5%	25.10	-17.0%
NotoNastaliqUrdu / fa-thelittleprince	55.91	53.78	-3.8%	47.86	-14.4%
NotoNastaliqUrdu / fa-words	59.80	57.17	-4.4%	50.60	-15.4%
NotoSansDevanagari / hi-words	16.50	16.46	-0.2%	15.91	-3.6%
Roboto / en-thelittleprince	4.18	4.17	-0.3%	4.07	-2.6%
Roboto / en-words	5.65	5.62	-0.5%	5.53	-2.1%
SourceSerifVariable / react-dom	45.37	45.22	-0.3%	44.49	-1.9%

next steps

This is encouraging: it demonstrates that sanitize is worthwhile independent of it enabling unsafe, and also shows that there are significant gains available if we do want to subsequently explore unsafe.

The main question remaining, for me, is just figuring out how to land this, in such a way as to minimize disruption for users. Some key considerations:

• the sanitize code is 'opt-in', so that it can be turned on for only specific codegen inputs.
• at least initially, methods on sanitized tables have the same signature as non-sanitized tables, which minimizes what has to break for the user. At some point in the future this can change and we can use better signatures for sanitized tables (not having to return results, since things are infallible) but we can decide when to make that change.
• tables that use sanitize can live beside tables that don't, so we can migrate incrementally.
• for tables that have sanitize impls, we generate FontRead impls that perform sanitization, so the public API doesn't really change. Is this good? in theory it might be nice if we could just rip all of this out and start again, but that isn't our reality, but it's possible that this deserves a bit of additional thought.

open questions

• if we want to skip sanitizing some portion of a graph (like with device/varidx tables in gpos) how do we do that?
• hb-sanitize does a bunch of stuff on top of just checking offsets and lengths: it checks for recursion depth, subtable counts, and has some other DOS protections. The current implementation isn't doing these things, although it is designed in such a way as to support them. Do we want to implement this before we use this on real tables, or not?

[behdad]

• if we want to skip sanitizing some portion of a graph (like with device/varidx tables in gpos) how do we do that?

I think we can annotate certain nodes in the graph as deferred-sanitize, and generate code to do it on access time. Is that feasible?

• hb-sanitize does a bunch of stuff on top of just checking offsets and lengths: it checks for recursion depth, subtable counts, and has some other DOS protections. The current implementation isn't doing these things, although it is designed in such a way as to support them. Do we want to implement this before we use this on real tables, or not?

Without recursion depth, the COLRv1 graph can easily cause tens of thousands or even more stack frames and crash the process. If you can pass down a mut sanitize_context, we can implement both a max-depth and work counter.

[cmyr]

• annotations to defer sanitize are definitely possible.
• there's a mut context in the patch already, intended to be able to handle these cases; i'm not too worried.