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

[Filip Roséen]

Hi Salah,

My previous email must have been written my a zombie that took over my
fingers, sorry about the weird wording and typos.

On 2017-03-12 01:47, Salah-Eddin Shaban wrote:

> >  +    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.

Hmm, not sure why I missed that particular `if`; sorry.

I would, however, argue that the limitation to `{ 400, 700 }` is just
that; a limitation. Maybe the implementation will be easier to read if
you move the usage of `b_bold` to *inside* the if-condition (since it
is only being used there), 

> >  +    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.

Oh, right; sorry.

> >  +    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.

Ops, the example is supposed to read like the below (note the lack of
separate end tag for `<font>`). The explanation was correct, the
sample (thrown in last minute as I at first didn't think it was
necessary).

    <familyset version="42">
      <family name="sans-serif">
         <font weight="700" style="normal"/>
       </family>
      <family name="monospace>
        <font weight="400" style="normal">DroidSansMono.ttf</font>
      </family>
    </familyset>

In either case; there are cases like the above that will end up in
weird behavior, and there are of course also cases where an unexpected
tag till cause the parsing to result in **very** weird data.

> 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.

It is not hard to protect continued parsing, nor do I find it
particularly good that the "trying" means that following parsed
elements are treated wrong.
 
> > 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.

In a well-formed file, sure; there should only be one. We should
however **never** trust that input data (because there is, of course,
nothing that states that it is always well-formed).

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

I would certainly not call it too paranoid, especially not that when
it results in a leak, and is really easy to protect against.

Pass the relevant pointer to `free` before assigning `ToLower` (as the
behavior will always be well-defined), or add a `NULL`-check.

Given what you wrote about `*psz_val`, I do not see why it is too
paranoid in this case (where it matters), but not in the other (where
it does not).

Best Regards,\
Filip
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170312/09a1015d/attachment.html>