+1 to the changes you have here. Comments on the upstream issues in-line.
-phil. On 12/14/2016 01:45 AM, Lindenmaier, Goetz wrote:
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?
There is about a 90% chance I will push an updated harfbuzz into JDK9 So your changes will get blown away But if you get them into upstream soon then I'd pull them in that way. If upstream says no .. then I would not want to maintain these in JDK.
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.
Ok.
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?
It still might. Again we like to keep these libraries up-to-date.
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.)
