[OpenJDK 2D-Dev] RFR(S): 8171248: Minor HarfBuzz fixes to pacify Coverity code scan

[Behdad Esfahbod]

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.simonis at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20161218/2c7ea918/attachment.html>