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

[alexey menkov]

looks ok

--alex

On 25.09.2015 21:14, Sergey Bylokhov wrote:
> 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
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>