[OpenJDK 2D-Dev] RFR(M): 8170798: Fix minor issues in java2d and sound coding.

[Lindenmaier, Goetz]

Hi Phil, 

thanks for looking at my change!
Updated webrev:
http://cr.openjdk.java.net/~goetz/wr16/8170798-java2d_sound/webrev.02/

Best regards,
  Goetz.

> (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.
I'll move to client.

> (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.
OK, I removed these changes.  But submitting upstream will probably mean
that I don't get the fixes into jdk9 any more.

>  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:
> 
> 	Changes in detail:
>
> 	hb-ot-font.cc 
> 	Looks like assignment instead of compare. Use extra if().
> This is a stylistic change. You will need to convince Behdad.
Yes.  The code scan complains about it, but it was correct.  But I would 
like to get it fixed.  It helps to get these false positives out of the way, as
we have to do these scans over and over again, and maybe other users of
openJdk, too.

> 	hb-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.
OK, I'll try to run the scan on the harfbuzz sources and fix stuff there.
But I don't think I'll get any changes I make in harfbuzz into jdk9 at 
this stage. So shouldn't I push it here, anyways?

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

> 	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.
Sorry, the comment is mixed up.  I added the size entry to the enum for the conState issue.
The array size OpenTypeLayoutEngine::scriptTags[scriptCodeCount] is needed because
OpenTypeLayoutEngine::scriptTags is accessed with an index that runs to scriptCodeCount.
That's the case because it's dual to indicClassTables if I remember correctly.
See OpenTypeLayoutEngine::getScriptTag() that potentially accesses scriptTags[] at
scriptCodeCount-1.

> 	jctrans.
> 	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.
Ok, I'll try that.  But that leaves the same issue: it will not reach jdk9 anymore?

> 	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.
Fixed.  (I thought it's cheaper to initialize only the rest, and more simple
for the C-compiler to remove where useless.)