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 > >
