RFR: 8282578: AIOOBE in javax.sound.sampled.Clip [v2]

[Alexander Zuev]

On Wed, 7 Dec 2022 15:15:34 GMT, Alexander Zvegintsev <azvegint at openjdk.org> wrote:

>> Alexander Zuev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains five additional commits since the last revision:
>> 
>>  - Got rid of the try/catch and using the proper per-case data length
>>    analysis.
>>  - Merge branch 'master' into JDK-8282578
>>  - Merge branch 'master' of https://github.com/azuev-java/jdk into JDK-8282578
>>  - Merge branch 'master'
>>  - 8282578: AIOOBE in javax.sound.sampled.Clip
>>    
>>    Surround SysEx message processing block with try/catch allowing MIDI subsystem to
>>    attempt to ingest the rest of the file.
>>    
>>    Added test case.
>
> src/java.desktop/share/classes/com/sun/media/sound/SoftMainMixer.java line 406:
> 
>> 404:                                     int ix = 0;
>> 405:                                     for (int j = 6; j < data.length - 1; j += 2) {
>> 406:                                         destinations[ix] = data[j] & 0xFF;
> 
> Here is another possible AIOOOBE
> 
> e.g. if `data` length is 8, we will have `destination` and `ranges` length of 0:
> 
> 
> int[] data = new int[8];
> 
> System.out.println("data len " + data.length);
> if (data.length < 7) {
>     System.out.println("Prevent");
>     return;
> }
> 
> int newSize = (data.length - 7) / 2;
> System.out.println("new size " + newSize);
> 
> int[] destinations = new int[newSize];
> int[] ranges = new int[newSize];
> int ix = 0;
> for (int j = 6; j < data.length - 1; j += 2) {
>     System.out.println("index %d %d".formatted(j, ix) );
>     destinations[ix] = data[j] & 0xFF;
>     System.out.println("index " + (j + 1));
>     ranges[ix] = data[j + 1] & 0xFF;
>     ix++;
> }
> 
> 
> Same applies to similar cases below.

Ok, i think there are 3 places total with this possibility when increment goes by 2 so fixed them all.

> src/java.desktop/share/classes/com/sun/media/sound/SoftMainMixer.java line 462:
> 
>> 460:                         case 0x0A:  // Key Based Instrument Control
>> 461:                         {
>> 462:                             if (data.length < 7 || (data[4] & 0xFF) != 01) {
> 
> Neat: `0x01`

Ok.

> src/java.desktop/share/classes/com/sun/media/sound/SoftTuning.java line 114:
> 
>> 112:                     //if (!checksumOK2(data))
>> 113:                     //    break;
>> 114:                     if (data.length < 406) {
> 
> At first glance it looks like a magic numbers to me.<br>
> for better readability we could declare `r` before this check and use `128*3 + r`

Makes sense, fixed.

> src/java.desktop/share/classes/com/sun/media/sound/SoftTuning.java line 136:
> 
>> 134:                     }
>> 135:                     int ll = data[6] & 0xFF;
>> 136:                     if (data.length < ll * 4 + 8) {
> 
> Shouldn't it be `ll * 4 + 7` == `ll * 4 + r`?

Yes.

> src/java.desktop/share/classes/com/sun/media/sound/SoftTuning.java line 231:
> 
>> 229:                 {
>> 230:                     // http://www.midi.org/about-midi/tuning-scale.shtml
>> 231:                     if (data.length < 21) {
> 
> `< 20`? 
> 
> 
> int[] data = new int[20];
> for (int i = 0; i < 12; i++) {
>     System.out.println(data[i + 8]);
> }

Yes. Math is hard.

-------------

PR: https://git.openjdk.org/jdk/pull/9016