[vlc-devel] [PATCH 1/1] Freetype: support Android's new fonts.xml

Salah-Eddin Shaban salah at videolan.org
Sun Mar 12 00:47:17 CET 2017


On Sat, Mar 11, 2017 at 10:34 PM, Filip Roséen <filip at atch.se> wrote:
> Hi Salah-Eddin,
>

Hello Filip,

> This reply contains my immediate reactions to the patch in question, and
> some thoughts expressed might be unnecessary nit-picky (or safe to ignore).
> I am currently very tired and therefore apologize in advance if a comment is
> missing some crucial piece of information (rendering it as
> false/unnecessary).
>

It's OK :)

>  +    while( ( psz_attr = xml_ReaderNextAttr( p_xml, &psz_val ) ) )
>  +    {
>  +        if( !strcasecmp( "weight", psz_attr ) && psz_val && *psz_val )
>  +            i_weight = atoi( psz_val );
>
> The above check of *psz_val is redundant as atoi will return 0 on an empty
> (or ill-formed) string.
>

I do that a lot actually. If a check is necessary in some situations
and not in others I tend to leave it unless there's a reason not to.
Otherwise it gets distracting.

But that's just me.

>  +    if( i_weight == 700 )
>  +        b_bold = true;
>
> As a font can be either bold or not-bold through NewFont, it might make
> sense to check whether i_weight is greater-than-or-equal to 700 (as 701
> would not result in a non-bold font).
>

Since we're only adding fonts with a weight of 400 and 700 it makes no
difference.

>  +    if( i_weight == 400 || i_weight == 700 )
>  +        if( asprintf( &psz_fontfile, "%s/%s", ANDROID_FONT_PATH, psz_val )
> < 0
>  +         || !NewFont( psz_fontfile, 0, b_bold, b_italic, p_family ) )
>  +            return VLC_ENOMEM;
>  +
>  +    return VLC_SUCCESS;
>
> Leak of the asprintf-allocated resource referred to by psz_fontfile.
>

No. NewFont takes ownership of psz_fontfile.

>  +    while( ( i_type = xml_ReaderNextNode( p_xml, &psz_val ) ) > 0 )
>  +    {
>  +        switch( i_type )
>  +        {
>  +        case XML_READER_STARTELEM:
>  +            if( !strcasecmp( "font", psz_val ) )
>  +                if( Android_ParseFont( p_filter, p_xml, p_family ) ==
> VLC_ENOMEM )
>  +                    return VLC_ENOMEM;
>
> There is the possibility that the above call to Android_ParseFont consumes
> the XML_READER_ENDELEM for </family> if the file contains unexpected data
> (such as the below, contrived example).
>
> <familyset version="42">
>   <family name="sans-serif">
>      <font weight="700" style="normal"></font>
>    </family>
>   <family name="monospace>
>     <font weight="400" style="normal">DroidSansMono.ttf</font>
>   </family>
> </familyset>
>
> As familyset.family.font[weight="700"] does not contain a XML_READER_TEXT
> node, Android_ParseFont would return VLC_EGENERIC, but as you only check for
> VLC_ENOMEM the affected loop would keep running, eventually adding
> DroidSansMono.ttf to the family named sans-serif instead of monospace.
>

Android_ParseFont calls xml_ReaderNextNode only once, so in this
particular example </family> will not be consumed.

That said, it's not hard to think of examples of invalid xml data
where the result would be as you describe. VLC would report in such
cases that the file is invalid and try to continue parsing it.

> If a node, for some reason, contains attributes such as to="..." and
> To="..." the above would leak acquired resources (as xml-attributes are
> case-sensitive).
>

Same is true for two lowercase to's, or two uppercase.
Aliases in fonts.xml contain only one.

We could check if psz_dest is NULL before calling ToLower, but that
seems too paranoid to me.

> Should this function really return VLC_SUCCESS if all calls to
> Android_ParseSystemFonts fail (for other reason then VLC_ENOMEM)?
>
>       return VLC_SUCCESS;
>

Yes. At the very least the Freetype module will be able to use the
hard-coded default font file and any font attachments that happen to
be present.

>  -        if( asprintf( &psz_name, FB_NAME"-%02d",
>  +        if( asprintf( &psz_name, FB_NAME"-%04d",
>
> I am not familiar with the inner workings of text_renderer/freetype, but is
> there a particular reason for the padding?
>

No, it's just aesthetic.


More information about the vlc-devel mailing list