Re: WOFF2 failure

If an API is wrong, remove it and break others' code.  That's better than
shipping buggy API.  We had the same in ICU LayoutEngine and added new API,
for years we had to hunt down bugs caused by clients still using old API,
whereas if we have had broken their build they would have fixed easily,
saving debugging time.

On Wed, Sep 21, 2016 at 10:37 PM, Roderick Sheeter <rsheeter@google.com>
wrote:

> I updated the comment to be more aggressive in asserting which to use/not
> use. Better late than never :D
>
> On Wed, Sep 21, 2016 at 2:09 PM, Ned Holbrook <ned@apple.com> wrote:
>
>> Thanks for the clarification. I’m pretty sure my colleague thought the
>> new API was merely a convenience, otherwise we would have picked the
>> "right" one before.
>>
>> On Sep 21, 2016, at 1:51 PM, Roderick Sheeter <rsheeter@google.com>
>> wrote:
>>
>> Sorry, that's my fault. I didn't want to remove the old API as doing so
>> would break anyone using it and the repo is public so it's not easy to do a
>> sweeping update of all consumers.
>>
>> As Khaled suggests, please use this (from https://github.com/google/woff
>> 2/blob/master/src/woff2_dec.h):
>>
>> // Decompresses the font into out. Returns true on success.
>> // Works even if WOFF2Header totalSfntSize is wrong.
>> bool ConvertWOFF2ToTTF(const uint8_t *data, size_t length,
>>                        WOFF2Out* out);
>>
>> And avoid this; it's more fragile:
>>
>> // Decompresses the font into the target buffer. The result_length should
>> // be the same as determined by ComputeFinalSize(). Returns true on
>> successful
>> // decompression.
>> bool ConvertWOFF2ToTTF(uint8_t *result, size_t result_length,
>>                        const uint8_t *data, size_t length);
>>
>> Rod S
>>
>> On Wed, Sep 21, 2016 at 12:20 PM, Khaled Hosny <khaledhosny@eglug.org>
>> wrote:
>>
>>> LOL, I actually first wrote a much detailed answer then thought it is
>>> too newbish and discarded it.
>>>
>>> Yes, you will need to use the new API that uses the WOFF2Out object as
>>> it does not suffer from the limitations of the old API WRT trusting
>>> totalSfntSize and origLength.
>>>
>>> Regards,
>>> Khaled
>>>
>>> On Wed, Sep 21, 2016 at 11:31:24AM -0700, Ned Holbrook wrote:
>>> > Thanks, but you’ll have to be a bit more explicit for us poor
>>> newcomers. :) The fix in this case would be to use the ConvertWOFF2ToTTF()
>>> overload taking a WOFF2Out object?
>>> >
>>> > > On Sep 21, 2016, at 11:25 AM, Khaled Hosny <khaledhosny@eglug.org>
>>> wrote:
>>> > >
>>> > > On Wed, Sep 21, 2016 at 11:11:12AM -0700, Ned Holbrook wrote:
>>> > >> This is regarding <http://test.chrislewis.codes/woff2/>, found on <
>>> http://typedrawers.com/discussion/1775/browser-ots-rejectio
>>> ns-for-woff2-but-not-woff>; the WOFF2 for the second line of the test
>>> page is not being used.
>>> > >>
>>> > >> I stepped through I discovered the failure is due to this code in
>>> woff2_dec.cc:
>>> > >>
>>> > >>    if (PREDICT_FALSE(static_cast<uint64_t>(table.dst_offset +
>>> table.dst_length)
>>> > >>> out->Size())) {
>>> > >>      return FONT_COMPRESSION_FAILURE();
>>> > >>    }
>>> > >>
>>> > >> At the point of failure:
>>> > >>    (lldb) p out->Size()
>>> > >>    (size_t) $7 = 405520
>>> > >>    (lldb) p table.dst_offset + table.dst_length
>>> > >>    (unsigned int) $8 = 417149
>>> > >>
>>> > >> Now, Khaled commented on the TypeDrawers page to suggest this check
>>> is
>>> > >> overly strict. Does this mean the reference implementation needs to
>>> be
>>> > >> fixed?
>>> > >
>>> > > See:
>>> > > https://github.com/google/woff2/pull/48
>>> > >
>>> > > Regards,
>>> > > Khaled
>>>
>>>
>>
>>
>