<div dir="ltr">Thanks Volker. Fixed upstream. I'll make a release in the next couple of days.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Dec 14, 2016 at 1:15 PM, Volker Simonis <span dir="ltr"><<a href="mailto:volker.simonis@gmail.com" target="_blank">volker.simonis@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
can I please have a review for the following small changes which fix<br>
some Coverity code scan issues in the Harfbuzz library:<br>
<br>
<a href="https://bugs.openjdk.java.net/browse/JDK-8171248" rel="noreferrer" target="_blank">https://bugs.openjdk.java.net/<wbr>browse/JDK-8171248</a><br>
<a href="http://cr.openjdk.java.net/~simonis/webrevs/2016/8171248/" rel="noreferrer" target="_blank">http://cr.openjdk.java.net/~<wbr>simonis/webrevs/2016/8171248/</a><br>
<br>
These changes only make sense if they are also accepted in the<br>
upstream HarfBuzz repository. I've therefore send out a pull request<br>
with the same changes and kindly requested Behdad to accept them<br>
upstream:<br>
<br>
<a href="https://github.com/behdad/harfbuzz/pull/377" rel="noreferrer" target="_blank">https://github.com/behdad/<wbr>harfbuzz/pull/377</a><br>
<br>
Following are the details of the fix:<br>
<br>
we regularly run Coverity code scans on the OpenJDK sources and<br>
recently discovered two issues with HarfBuzz. While the discovered<br>
issues are not real errors, we think that fixing them my be<br>
nevertheless worthwile in order to increase the readability of the<br>
source code.<br>
<br>
We just wanted to ask, if you are willing to accept these changes in<br>
the upstream HarfBuzz repository because only then it would make sense<br>
to also fix them in the OpenJDK copy of HarfBuzz.<br>
<br>
The first issue found by Coverity is the last of the following four<br>
lines from src/hb-ot-font.cc:<br>
<br>
if (!subtable) subtable = cmap->find_subtable (0, 2);<br>
if (!subtable) subtable = cmap->find_subtable (0, 1);<br>
if (!subtable) subtable = cmap->find_subtable (0, 0);<br>
if (!subtable)(subtable = cmap->find_subtable (3, 0)) && (symbol = true);<br>
<br>
>From the whole context it really took me some time to understand that<br>
'symbol' should only be set to true if 'subtable' is set from<br>
'cmap->find_subtable (3, 0)'. Coverity reports an "assignment instead<br>
of compare" which is a false positive, but we think the could would be<br>
much more readable if changed to look as follows:<br>
<br>
if (!subtable)<br>
{<br>
subtable = cmap->find_subtable (3, 0);<br>
if (subtable) symbol = true;<br>
}<br>
<br>
The second issue is related to the following definition in<br>
src/hb-ot-layout-gpos-table.<wbr>hh:<br>
<br>
ValueFormat valueFormat1; /* Defines the types of data in<br>
* ValueRecord1--for the first glyph<br>
* in the pair--may be zero (0) */<br>
ValueFormat valueFormat2; /* Defines the types of data in<br>
* ValueRecord2--for the second glyph<br>
* in the pair--may be zero (0) */<br>
<br>
Throughout hb-ot-layout-gpos-table.hh, '&valueFormat1' is used as if<br>
it were an array of two ValueFormat objects. While extremely unlikely,<br>
a compiler could theoretically insert padding between 'valueFormat1'<br>
and 'valueFormat2' which would make the code incorrect. We would<br>
therefore propose to simply change the previous definiton into a real<br>
array:<br>
<br>
ValueFormat valueFormat[2]; /* [0] Defines the types of data in<br>
* ValueRecord1--for the first glyph<br>
* in the pair--may be zero (0) */<br>
/* [1] Defines the types of data in<br>
* ValueRecord2--for the second glyph<br>
* in the pair--may be zero (0) */<br>
<br>
and change the code which uses 'valueFormat' accordingly.<br>
<br>
Thank youand best regards,<br>
Volker<br>
</blockquote></div><br></div>