<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="Content-Style-Type" content="text/css" />
<meta name="generator" content="pandoc" />
<title></title>
<style type="text/css">code{white-space: pre;}</style>
</head>
<body>
<p>Hi Salah,</p>
<p>My previous email must have been written my a zombie that took over my fingers, sorry about the weird wording and typos.</p>
<p>On 2017-03-12 01:47, Salah-Eddin Shaban wrote:</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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).</code></pre>
</blockquote>
<pre><code> Since we're only adding fonts with a weight of 400 and 700 it makes no
difference.</code></pre>
</blockquote>
<p>Hmm, not sure why I missed that particular <code>if</code>; sorry.</p>
<p>I would, however, argue that the limitation to <code>{ 400, 700 }</code> is just that; a limitation. Maybe the implementation will be easier to read if you move the usage of <code>b_bold</code> to <em>inside</em> the if-condition (since it is only being used there),</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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.</code></pre>
</blockquote>
<pre><code> No. NewFont takes ownership of psz_fontfile.</code></pre>
</blockquote>
<p>Oh, right; sorry.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> + 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.</code></pre>
</blockquote>
<pre><code> Android_ParseFont calls xml_ReaderNextNode only once, so in this
particular example </family> will not be consumed.</code></pre>
</blockquote>
<p>Ops, the example is supposed to read like the below (note the lack of separate end tag for <code><font></code>). The explanation was correct, the sample (thrown in last minute as I at first didn’t think it was necessary).</p>
<pre><code><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></code></pre>
<p>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 <strong>very</strong> weird data.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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.</code></pre>
</blockquote>
<p>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.</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> 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).</code></pre>
</blockquote>
<pre><code> Same is true for two lowercase to's, or two uppercase.
Aliases in fonts.xml contain only one.</code></pre>
</blockquote>
<p>In a well-formed file, sure; there should only be one. We should however <strong>never</strong> trust that input data (because there is, of course, nothing that states that it is always well-formed).</p>
<blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;color:#500050">
<pre><code> We could check if psz_dest is NULL before calling ToLower, but that
seems too paranoid to me.</code></pre>
</blockquote>
<p>I would certainly not call it too paranoid, especially not that when it results in a leak, and is really easy to protect against.</p>
<p>Pass the relevant pointer to <code>free</code> before assigning <code>ToLower</code> (as the behavior will always be well-defined), or add a <code>NULL</code>-check.</p>
<p>Given what you wrote about <code>*psz_val</code>, I do not see why it is too paranoid in this case (where it matters), but not in the other (where it does not).</p>
<p>Best Regards,<br />
Filip</p>
</body>
</html>