I have created a bug on the upgrade
https://bugs.openjdk.java.net/browse/JDK-8171456
I don't expect to get to this until January as I'll be out until then
and there is
some internal process to be navigated. And of course Behdad needs to make
the release too.
So if you can't wait until then I don't object to it being pushed but it
also isn't all that critical/urgent either ..
-phil.
On 12/19/2016 12:31 AM, Volker Simonis wrote:
Thanks Behdad!
@Phil: what do you think, can I push this now that it was committed
upstream? Or should I wait until you pull in a new HarfBuzz release?
In that case, do you already have a bug open for that task such that I
can link it to the current issue?
Thank you and best regards,
Volker
On Sun, Dec 18, 2016 at 8:23 AM, Behdad Esfahbod <beh...@google.com> wrote:
Thanks Volker. Fixed upstream. I'll make a release in the next couple of
days.
On Wed, Dec 14, 2016 at 1:15 PM, Volker Simonis <volker.simo...@gmail.com>
wrote:
Hi,
can I please have a review for the following small changes which fix
some Coverity code scan issues in the Harfbuzz library:
https://bugs.openjdk.java.net/browse/JDK-8171248
http://cr.openjdk.java.net/~simonis/webrevs/2016/8171248/
These changes only make sense if they are also accepted in the
upstream HarfBuzz repository. I've therefore send out a pull request
with the same changes and kindly requested Behdad to accept them
upstream:
https://github.com/behdad/harfbuzz/pull/377
Following are the details of the fix:
we regularly run Coverity code scans on the OpenJDK sources and
recently discovered two issues with HarfBuzz. While the discovered
issues are not real errors, we think that fixing them my be
nevertheless worthwile in order to increase the readability of the
source code.
We just wanted to ask, if you are willing to accept these changes in
the upstream HarfBuzz repository because only then it would make sense
to also fix them in the OpenJDK copy of HarfBuzz.
The first issue found by Coverity is the last of the following four
lines from src/hb-ot-font.cc:
if (!subtable) subtable = cmap->find_subtable (0, 2);
if (!subtable) subtable = cmap->find_subtable (0, 1);
if (!subtable) subtable = cmap->find_subtable (0, 0);
if (!subtable)(subtable = cmap->find_subtable (3, 0)) && (symbol =
true);
From the whole context it really took me some time to understand that
'symbol' should only be set to true if 'subtable' is set from
'cmap->find_subtable (3, 0)'. Coverity reports an "assignment instead
of compare" which is a false positive, but we think the could would be
much more readable if changed to look as follows:
if (!subtable)
{
subtable = cmap->find_subtable (3, 0);
if (subtable) symbol = true;
}
The second issue is related to the following definition in
src/hb-ot-layout-gpos-table.hh:
ValueFormat valueFormat1; /* Defines the types of data in
* ValueRecord1--for the first
glyph
* in the pair--may be zero (0) */
ValueFormat valueFormat2; /* Defines the types of data in
* ValueRecord2--for the second
glyph
* in the pair--may be zero (0) */
Throughout hb-ot-layout-gpos-table.hh, '&valueFormat1' is used as if
it were an array of two ValueFormat objects. While extremely unlikely,
a compiler could theoretically insert padding between 'valueFormat1'
and 'valueFormat2' which would make the code incorrect. We would
therefore propose to simply change the previous definiton into a real
array:
ValueFormat valueFormat[2]; /* [0] Defines the types of data in
* ValueRecord1--for the first
glyph
* in the pair--may be zero (0) */
/* [1] Defines the types of data in
* ValueRecord2--for the second
glyph
* in the pair--may be zero (0) */
and change the code which uses 'valueFormat' accordingly.
Thank youand best regards,
Volker
