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

[Phil Race]

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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20161213/57588c3a/attachment.html>