The duplication might be hidden behind a configuration macro...
If the patch is accepted, I am in favor of this being not default compiled into the library for the reasons that Behdad lays out above.
Thanks for the review and merging, Werner.
I thought about that for a while too: I think the fuzzer was smarter and found the loophole. I tried the - 4 in this spot, but that didn't do it. The reason is: This check in L240 is checking against whether the BaseGlyphRecord
array potentially extends past the end of the whole COLR table. It does not check and cannot check whether the BaseGlyphRecord
array extends into other, non-BaseGlyphRecord-Array parts of the table - for example the region where we have just Paint*
tables. So then, there is a space between the "correct" end of the BaseGlyphRecord
array and the end of the COLR table. And the binary search can travel into those regions and the fuzzer can write arbitrate glyph id values there and thus control the binary search - and only then new pointer destinations are created which extend outside the COLR table. That's my understanding.
CC @bungeman - PTAL, Ben, does this look good to you?
Fixes https://bugs.chromium.org/p/chromium/issues/detail?id=1505216
Dominik Röttsches (10c242b2) at 02 Jan 16:47
[sfnt] Guard access in 'COLR' v1 glyph binary search
... and 948 more commits
@bungeman see also @behdad 's comment here: !174 (comment 2155552)
Alexei, I would kindly invite you to lead the discussion in a way that is constructive, leading to a result that is beneficial to the project. You certainly have expertise in this area - we'd be happy to see you use it in a helpful way. Here, a new contributor to FreeType is trying to deliver a fix for FreeType which is affecting Chrome users, let's be welcoming towards that.
FT_Fixed
is typedef'ed to be long
, which for the Chromium project amounts to 48.16 almost on all platforms we build for. We expect values to fit into 16.16, 32bit, for everything. But on 64 bit platforms, there's no issue even if our evaluation is wrong, we have spare space on those. Whether it always fits, is a yes/no question as you claim only if we would know all existing and future fonts. In the discussion on the issue and consulting with @behdad, we think it's unlikely.
Taking a practical approach, keeping internal 64bit precision covers even an unexpected outlier situation, if these values would overflow. I think @moonira's example is theoretically possible and keeping the internal 64bit emulation avoids issues in that case. We will leave that up to you and Werner if you want it removed or not, please let us know.
It's true that for the return value, we only need to return temp.lo
and we'll update that part and update the name of the function.
FWIW from looking at what these values typically are used for (compare #1257 (comment 2114118)) we do expect them to fit into 32bit.
I suspect we only need the non rounding version of this. Are there locations where we actually want to shift and truncate? What I am trying to say: Can we remove the round argument and only have one version of this function?
We're not shifting the value either, so the value is larger. Please revise the comment. Function name is not updated yet in the comment. Please check the description as well. I think given that we don't shift, the formula might not be correct.
(We had problems with false positives from Gitlab spam filtering, where to report that?)
@behdad could not submit a comment, but says in chat:
"Proposed approach sounds good. Off the top of my head I don't see any of those callsites needed more than 16.16 bits. Ironically, the reason we added 32bit values to the VarStore was for COLRv1. I'll try to remember why that was."
To which I can respond: We introduced 32bit deltas in the VarStore for COLRv1 because COLRv1 has 32bit Fixed
values for example in the Affine2x3
and F2DOT14
types in PaintRadialGradients
and we needed the fractional precision deltas to apply to those.
As much as I can tell, we have the following call sites for tt_var_get_item_delta
ttgxvar.c
, function tt_apply_mvar()
ttgxvar.c
, function tt_hvadvance_adjust()
ttgxvar.c
, function ft_var_to_normalized()
ttcolr.c
, function get_deltas_for_var_index_base()
From this list, I dare to conclude there's only very small risk of failing to represent a large value for the regular use situations of these units. While on the other side, we do have actual users reporting loss of precision for COLRv1.
Hence, I suggest to
FT_ItemVarDelta
typedef from FT_Int32
to FT_Fixed
,tt_var_get_item_delta
function to not round and truncate to integer valuesFor compatibility reasons, we might want to do 4. to any callsite. But I think in the case of AVAR2, we can already do it, before use picks up.
Are there any objections to this approach?
CC @behdad
@moonira - Could you try to update the draft change to reflect the above? In other words, remove the extra bool round
argument and always have tt_var_get_item_delta
produce the required precision, but updating the existing callsites?
For context, this seems to be identical to a historical issue http://crbug.com/856023 - where the precision for glyph deltas was not high enough. Compare previous change 1178227b in which @wl improved precision for applying glyph deltas.
The question remains is if we need int32s sizes in some situations for tt_var_get_item_delta
or if we can always use FT_Fixed
.
Yes, for the purpose of the gradients, I don't think we necessarily need the bits 33 and above. And I think, for most (tbd?) of the tt_var_get_item_delta
situations we do not need such high values either. So one fix, similar to Munira's fix above, may be introduce a separate multiplication function for use from tt_var_get_item_delta
that preserves more precision without rounding and truncating, then use that in tt_var_get_item_delta
and enable it to return a fractional delta value.
For the gradient types, the values are in font units - so I'd say the range is perhaps something in the range of negative to positive 2-3x UPEM - but to avoid jittering we need maybe a 16th of a Pixel or more precision (for larger font sizes). The change above is not the final proposal on the fix. It can take other forms, hence my comment about whether we can work with a scale factor.
moonira/freetype@350c4723 is a change that addresses the problem and illustrates the issue - but has issues with truncating the high part of the product in FTMulAddFix_WithoutRounding
.
CC @bungeman
This particularly applies to p0
, p1
, and p2
of linear gradients and for c0
, c1
, r0
, r1
for radial gradients for example. For these ftcolor.h
external FT API has FT_PaintLinearGradient
and FT_PaintSweepGradient
which provide FT_Vector or FT_Fixed coordinates.
But in the current implementation, we do not produce any fractional precision for them. That's because tt_var_get_item_delta
produces an FT_Int32
return values. Multiple deltas for a set of gradient coordinates are fetched in ttcolr.c
in get_deltas_for_var_index_base
. So the resulting delta that is applied to a p0
, p1
etc. is a rounded integer value. Visually, that then causes jitter.
So for these gradient coordinates we internally need to produce higher precision values internally. Not quite sure yet how to that without a too intrusive change. Perhaps we can work with a scale factor in tt_var_get_item_delta
?
Thanks very much Ben, good find and improvement to avoid this problem in the future.