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

[Sergey Bylokhov]

Hi, Phil.
The new version is here:
http://cr.openjdk.java.net/~serb/8135160/webrev.01

In case of negative value we assume an EOF.

On 25.09.15 19:31, Phil Race wrote:
> 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
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>


-- 
Best regards, Sergey.