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

[Volker Simonis]

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