Hi Goetz,
Comments in-line but first some general points.
(1) I notice you prepared the webrev against jdk9/dev.
It should be prepared against jdk9/client - which is where it
should also be pushed.
(2) However most of these changes are in a 3rd party imported library.
We don't edit these unless there is a good reason. Instead we
would report
them to up-stream and import next time we upgrade.
So only a subset of these make sense to fix in JDK .. otherwise
we'll just blow
them away when we upgrade.
In particular this means the changes to
(a) harfbuzz
(b) littlecms
which are actively maintained externally and we upgrade periodically.
Local changes may make sense in
(c) ICU layout
(d) libjpeg6b
since these are at a different stage in their life
Having said that see the comments in-line.
-phil
On 12/08/2016 11:57 PM, Lindenmaier, Goetz wrote:
Hi,
This change fixes some minor issues found in our code scans.
I hope this correctly addresses corelib and serviceability issues.
Please review:
http://cr.openjdk.java.net/~goetz/wr16/8170798-java2d_sound/webrev.01/
<http://cr.openjdk.java.net/%7Egoetz/wr16/8170798-java2d_sound/webrev.01/>
Best regards,
Goetz.
Changes in detail:
hg-ot-font.cc
Looks like assignment instead of compare. Use extra if().
This is a stylistic change. You will need to convince Behdad.
hg-ot_layout-gpos-table.hh
valueFormat is passed to apply(), where it is used as an array with
two elements:
line 621: valueFormats[1].get_len();
It was correct as there are actually two fields in the struct that
have the
same layout as an array.
This also needs to go to the harfbuzz mailing list.
ScriptAndLanguageTags.cpp, ThaiShaping.cpp/.h
In ThaiShaping.cpp:307 conState is passed to getNextState() where it
is in the end used to index to thaiStateTable.
thaiStateTable has 52 elements. But conState is initialized to 0xFF ==
255 in ThaiShaping.cpp:296. This can result in an out-of-bounds access.
This is not entered unconditionally, so consState should be updated
before we reach there. However the check is harmless so should be fine.
OpenTypeLayoutEngine::scriptTags[scriptCodeCount] is accessed with
index < scriptCodeCount, but only contains scriptCodeCount-1 elements.
I added a size entry to the enums, and use that for sizing the array
and checking the size.
I am fairly sure you didn't include all of these changes in the webrev.
I see only the use for sizing the array
jctrans.c
if cinfo->entropy->encode_mcu resolves to encode_mcu_AC_first() it
will access MCU_buffer[0]. (jcphuff.c:487)
It is hard to prove with static analysis but it seems like the block
above does the initialisation.
So I don't strongly object to this but I also don't see that the case
for it is proven.
cmserr.c
Must check return value of ftell.
cmsgamma.c
Out/out/in are used as arrays in called function.
cmslut.c
Out[] may be used uninitialized.
cmstypes.c
Must check return value of Tell. The negative outcome should not be
passed to Seek.
cmsxform.c
Using uninitialized element of array wIn when calling *p->FromInput.
(The function pointer resolves to Pack1Byte.)
Using uninitialized element of array fIn when calling
*p->FromInputFloat. (The function pointer resolves to
PackDoublesFromFloat.)
Using uninitialized element of array fIn when calling
*p->FromInputFloat. (The function pointer resolves to
PackDoublesFromFloat.)
All of the above should be proposed to the upstream littlecms list
PLATFORM_API_LinuxOS_ALSA_Ports.c
Using uninitialized element of array controls when calling
*creator->newCompoundControl. (The function pointer resolves to
PORT_NewCompoundControl.)
So it seems like the elements at indices from controlCount to 10 may not
be initialised but I don't see how this would be a problem since
PORT_NewCompoundControl
has no hardwired "10" .. it just uses controlCount.
In any case this would seem better addressed by the same kind of memset
after stack allocation that you proposed for the jpeg case.
-phil.
