<Sound Dev> [9] Review Request: 8135160 Endless Loop in RiffReader

[Phil Race]

I think skip is *supposed* to return >=0 but we should protect against
it doing the wrong thing. Treat < 0  as 0 .. or EOF ..

-phil.

On 09/25/2015 09:22 AM, Sergey Bylokhov wrote:
> On 25.09.15 19:12, Phil Race wrote:
>>> long ret = Math.min(stream.skip(remaining), remaining);
>>
>> what is to stop stream.skip returning '-100' ?
>>
>> should you not check for negative ?
>
> This part of specification is unclear also. If the requested number of 
> bytes is positive, then can the skip method returns the negative value 
> or not. And it is unclear how the negative value should be processed.
>
>
>>
>> -phil.
>>
>>
>> On 09/24/2015 04:38 PM, Sergey Bylokhov wrote:
>>> Hello.
>>> Please review the fix for jdk9.
>>>
>>>     While working on some bugs in javasound, I found one issue which
>>> can cause an endless loop for some specific audio file. The bug is
>>> located in the com.sun.media.sound.RIFFReader.skipBytes() method,
>>> which incorrectly assumes that Inputstrem.skip() method should return
>>> -1 at the end of the stream. Note that the specification of the read()
>>> method has this assumption.
>>>
>>> The current implementation of the skip method has a loop which stops
>>> when the necessary amount of bytes are skipped or the -1 is returned.
>>>
>>> In the fix I change implementation of this loop:
>>>  - The check for the negative value is removed, because such checks
>>> are not specified.
>>>  - The check for the zero is enhanced using read() method. This is
>>> necessary, because the specification of the skip method mention that
>>> the zero can be returned if EOF occurred or nothing was skipped for
>>> some other reason and the user should try again.
>>>  - Note that the skip method can return more bytes than was requested
>>> if EOF occurred(at least this part of spec is unclear???), the check
>>> for this is also added(otherwise the avail variable can became 
>>> negative).
>>>
>>> One new test was added, it was used as initial reproducer. One existed
>>> test was updated.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8135160
>>> Webrev can be found at:
>>> http://cr.openjdk.java.net/~serb/8135160/webrev.00
>>>
>>>
>>> ps: Note that two review requests still active in the javasound:
>>>
>>> http://mail.openjdk.java.net/pipermail/sound-dev/2015-September/000330.html 
>>>
>>>
>>>
>>> http://mail.openjdk.java.net/pipermail/sound-dev/2015-September/000332.html 
>>>
>>>
>>>
>>>
>>
>
>