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 >
