[CFF] ByteStr issues...

[behdad]

This was brought to my attention thanks to ubsan on my local build:

../../../test/fuzzing/../subset/data/fonts/SourceHanSans-Regular.otf
../../src/hb-ot-cff-common.hh:152:6: runtime error: member call on misaligned address 0x0000022b17e1 for type 'CFF::ByteStr', which requires 8 byte alignment
0x0000022b17e1: note: pointer points here
 02 24 02  4c 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00
              ^ 
running subset fuzzer against ../../../test/fuzzing/../subset/data/fonts/SourceSansPro-Regular.otf
../../../test/fuzzing/../subset/data/fonts/SourceSansPro-Regular.otf
../../src/hb-ot-cff-common.hh:152:6: runtime error: member call on misaligned address 0x000002b4d363 for type 'CFF::ByteStr', which requires 8 byte alignment
0x000002b4d363: note: pointer points here
 02  5d 02 63 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00
              ^ 
running subset fuzzer against ../../../test/fuzzing/../subset/data/fonts/Mplus1p-Regular.ttf
../../../test/fuzzing/../subset/data/fonts/Mplus1p-Regular.ttf

That points to this piece of code:

      /* serialize data */ 
      for (unsigned int i = 0; i < byteArray.len; i++) 
      { 
↦       ByteStr  *dest = c->start_embed<ByteStr> (); 
↦       if (unlikely (dest == nullptr || 
↦       ↦             !dest->serialize (c, byteArray[i]))) 
↦         return_trace (false); 
      } 

which suggests that ByteStr is a font struct. However, the implementation is NOT. It has a pointer, constructors, etc. I don't understand how you can start_embed that. That's just plain wrong.

The convention is that CamelCase are font structs, whereas the rest are hb_name_spaced_t.

The ByteStr looks awfully like hb_bytes_t. I'm not sure what it's used for. Can you please explain your needs, so we can figure out what's the best design?

[blueshade7]

ByteStr is a holder for a section of bytes extracted from CFFIndex, so yes it’s like hb_bytes_t. At the beginning I tried to use hb_bytes_t as-is but had some difficulty in doing so which I can’t recall exactly. One was that I wanted unsigned char bytes instead of char bytes. Then ended up writing my own. Calling start_embed<ByteStr> () certainly looks wrong in retrospect.

I can try reimplementing on top of hb_bytes_t. Shall we name it hb_cff_bytes_t, or something? Its scope is in CFF namespace.

[behdad]

Thanks. I just added hb_ubytes_t that is unsigned. Is that enough? I prefer to avoid hb_cff_bytes_t if it can be designed into a generic struct.

[blueshade7]

It’s nice to see hb_array_t has sanitize (), but hb_ubytes_t::sanitize () fails to compile:

../src/hb-machinery.hh:356:42: error: type 'unsigned char' cannot be used prior to '::' because it has no members
    return this->check_range (base, len, T::static_size);

[behdad]

It’s nice to see hb_array_t has sanitize (), but hb_ubytes_t::sanitize () fails to compile:

Should be fixed now.

Another nice but possibly very confusing property of hb_array_t / hb_bytes_t is their overridden operator &, which returns pointer to data they point to. This allows for objects of those types with things like StructAfter<>.

[blueshade7]

ByteStr based on hb_ubytes_t seems working with some additional utility functions for my use. start_embed call removed and ByteStr will rename to byte_str_t.

Shall I rename ALL non-table structs likewise, such as CFF1TopDict_OpSerializer => cff1_top_dict_op_serializer_t? This is straightforward, but going to be substantial code changes (only name changes in CFF namespace for that matter).

[behdad]

ByteStr based on hb_ubytes_t seems working with some additional utility functions for my use. start_embed call removed and ByteStr will rename to byte_str_t.

Sounds good. I'm curious to see what other utilities you need there.

Shall I rename ALL non-table structs likewise, such as CFF1TopDict_OpSerializer => cff1_top_dict_op_serializer_t? This is straightforward, but going to be substantial code changes (only name changes in CFF namespace for that matter).

Would be nice, but I'm never strict about such big changes. Do it if you don't mind. There's no pull requests against the CFF code, so feel free to make huge changes while we can. :)

[behdad]

Is this fixed? @blueshade7

[blueshade7]

Yes as a part of PR #1507