[colr] Make rejecting paint-graph cycles in COLRv1 a compile-time option #272
Conversation
|
Thanks for doing that. Non owner LGTM, but I am wondering if when option |
It's probably still desirable to log a warning if a cycle is present (which means the paint graph is invalid, per spec, though it may still be partially rendered). And we'd still want to traverse the paint graph in order to check the paint records for other kinds of error, no? |
|
In my mind, it depends on performance and threat model considerations: From Chrome's perspective, I see the value in OTS in preventing downloading malicious fonts that would be passed to the system renderers DirectWrite and CoreText, as we don't know their fuzzing and security hardening status. For the open source libraries we use, FreeType and Harfbuzz - and soon Rust-based Fontations, we expect the libraries to be sufficiently hardened and fuzzed against handling a malicious font gracefully enough and without harm - one of the reasons we move to Rust parsing there is to make that safer - and Fontations will be safer than any OTS pre-checks would possibly be - because of language design and related safety. But since COLRv1 in FF and in Chrome is handled in internal rasterizers, I am not sure an extra pass needs to be done other than the on-the-fly sanity checks that FT and Fontations do. But perhaps I went off in the wrong direction, and you meant the warning mostly as a useful means to the page author? I am not sure how a page author would get into a situation with a COLRv1 font containing a cycle. I could only see that with a very fine-grained manual process. Tools like nanoemoji or other COLRv1 capable generators would usually not introduce such cycles. In other words, I would find that case unlikely and likely not worth the extra traversal overhead for every glyph. |
Agree, it's unlikely that a page author would inadvertently deploy a font containing cycles, and of course a malicious actor attempting to use cycles to break the rasterizer (for example) won't care about a warning. But the thing is, we want to traverse the paint graph anyway, in order to validate that the paint records and their transforms, color-lines, etc can be safely accessed; and we want to keep track of visited nodes in the graph so that we can avoid re-validating already-seen nodes. At that point, detecting cycles is essentially free, and for a general (standalone) validation tool we definitely should do this, even if browsers decide they don't need it. So I don't think there's much benefit to skipping cycle-detection altogether, and it would clutter the code with more conditionals. Just switching between a warning and an error when a cycle is detected is a trival option. (Of course, if a browser is sufficiently confident in the robustness of its COLR-table handling, there's always the option to pass it through without validating at all... In gecko, at least for now, the COLR processing is intended to be safe in the face of bad tables, but we also want to pre-validate them with OTS as a form of defence in depth.) @khaledhosny Curious if you have any opinion here - are you happy to merge this as it stands, or are there further adjustments we should include? |
No strong opinion here, but if you are happy with it then it is fine by me. |
|
OK, I think we should merge this as it stands for now; if further refinements seem desirable, we can explore them in followups. |
This will allow clients that handle cycles safely at rendering time to disable the OTS rejection.
Motivated by https://bugzilla.mozilla.org/show_bug.cgi?id=1870240.
(cc @drott)