[ft-devel] Re: Some public APIs using FT_Int/FT_UInt should be improved in future

[mpsuzuki]

Dear Werner,

Thank you for quick response!

On Wed, 24 Dec 2008 18:27:22 +0100 (CET)
Werner LEMBERG <address@hidden> wrote:
>> A) Some 32bit (or more, in future extension?) flags are
>>    interchanged by FT_UInt.
>>
>>    Example: FT_Get_Advance(), FT_Get_Advance() receive flags by
>>    FT_UInt type, but FT_ADVANCE_FLAG_FAST_ONLY (=0x20000000UL) etc
>>    cannot be passed by FT_UInt on 16bit system.
>>
>>    Although the extension from FT_UInt to portable 32bit integer
>>    makes 16bit systems troubled, I think it should be extended.  To
>>    keep ILP32/LP64 systems unchanged, using FT_UInt32 would be most
>>    appropriate.
>
>Please use FT_Long for FT_Get_Advance and FT_Get_Advances since they
>are new in version 2.3.8, so no compatibility problems can occur.  Are
>there other functions which have this problem?

Yet I cannot privide a concrete scenario that a problem arises
(and how to fix it), I've found the casts of load_flags in
FTC_ImageCache_Lookup() and FTC_SBitCache_LookupScaler().

Here I quote a part of FTC_SBitCache_LookupScaler()...
=============================================================
src/cache/ftcbasic.c
-------------------------------------------------------------
  typedef struct  FTC_BasicAttrRec_
  {
    FTC_ScalerRec  scaler;
    FT_UInt        load_flags;

  } FTC_BasicAttrRec, *FTC_BasicAttrs;

#define FTC_BASIC_ATTR_COMPARE( a, b )                                 \
          FT_BOOL( FTC_SCALER_COMPARE( &(a)->scaler, &(b)->scaler ) && \
                   (a)->load_flags == (b)->load_flags               )

#define FTC_BASIC_ATTR_HASH( a )                                   \
          ( FTC_SCALER_HASH( &(a)->scaler ) + 31*(a)->load_flags )


  typedef struct  FTC_BasicQueryRec_
  {
    FTC_GQueryRec     gquery;
    FTC_BasicAttrRec  attrs;

  } FTC_BasicQueryRec, *FTC_BasicQuery;

  [snip]

  FT_EXPORT_DEF( FT_Error )
  FTC_SBitCache_LookupScaler( FTC_SBitCache  cache,
                              FTC_Scaler     scaler,
                              FT_ULong       load_flags,
                              FT_UInt        gindex,
                              FTC_SBit      *ansbit,
                              FTC_Node      *anode )
  {
    FT_Error           error;
    FTC_BasicQueryRec  query;

  [snip]

    query.attrs.scaler     = scaler[0];
    query.attrs.load_flags = load_flags;
=============================================================

FTC_SBitCache_LookupScaler() takes FT_ULong load_flags
from the 3rd argument, and write it to FT_UInt load_flags
in the member of FTC_BasicAttrRec. It causes implicit cast
in 16bit and LP64 platforms.
An easy idea would be the extension of FTC_BasicAttrs->load_flags
from FT_UInt to FT_UInt32 (or FT_ULong), but yet I'm not fully
understanding the hash utilization in the cache subsystem and
difficult for me to write appropriate workaround. So my previous
patch just inserted the checks to warn dropped higher bits.

In addition, FT_Get_SubGlyph_Info() may have similar problem.
Current interface takes 2 pointers to FT_Int (p_arg1 & p_arg2).
In the case of TrueType font, the pointed values are used as
the index of points in TrueType glyph shape description.
Yet I've not fully checked the TrueType instruction spec,
I guess the number of points in TrueType would be 32bit,
not 16bit. I think the TrueType glyph shape description
including many many points (> 64k) is not pragmatic, but
theoretically it is possible, and passing 32bit integer by
p_arg1 & p_arg2 would be theoretically safer.

Except of the function APIs, as I mentioned about the type
of glyph index, I want to discuss the changes for some
public data types. I will describe them in detail, within
2009-Jan (sorry!).

Regards,
mpsuzuki