[vlc-devel] [PATCH] src: input/stream: fix buffer underflow by avoiding inconsistent state in error case

[David Fuhrmann]

Am 09.06.2014 um 20:51 schrieb David Fuhrmann <david.fuhrmann at gmail.com>:

> 
> Am 09.06.2014 um 20:14 schrieb Rémi Denis-Courmont <remi at remlab.net>:
> 
>> Le lundi 9 juin 2014, 19:56:18 david.fuhrmann at gmail.com a écrit :
>>> From: David Fuhrmann <dfuhrmann at videolan.org>
>>> 
>>> The error case i_pos > tk->i_end implies that
>>> p_sys->stream.i_offset > tk->i_end - tk->i_start. This is not allowed
>>> as it leads to unsigned underflows in several places. Thus, i_offset
>>> is set to a sane value.
>>> 
>>> The conversion to int in AStreamRefillStream is necessary to properly
>>> detect i_toread < 0, and to avoid the mentioned underflow.
>> 
>> I think you're better off eliminating any case that leads to this, because this 
>> is ostensibly bogus.
>> 
>>> 
>>> close #11488
>>> ---
>>> src/input/stream.c | 8 +++++---
>>> 1 file changed, 5 insertions(+), 3 deletions(-)
>>> 
>>> diff --git a/src/input/stream.c b/src/input/stream.c
>>> index 18e77e2..eafc7a2 100644
>>> --- a/src/input/stream.c
>>> +++ b/src/input/stream.c
>>> @@ -1261,8 +1261,10 @@ static int AStreamSeekStream( stream_t *s, uint64_t
>>> i_pos ) if( p_sys->stream.i_used < STREAM_READ_ATONCE / 2 )
>>>             p_sys->stream.i_used = STREAM_READ_ATONCE / 2;
>>> 
>>> -        if( AStreamRefillStream( s ) && i_pos >= tk->i_end )
>>> +        if( AStreamRefillStream( s ) && i_pos >= tk->i_end ) {
>>> +            p_sys->stream.i_offset = tk->i_end - tk->i_start;
>> 
>> This looks fishy. RefillStream should never let i_offset point outside the buffer 
>> to begin with.
> 
> Hi,
> 
> In this situation, i_offset is alread pointing outside the buffer before AStreamRefillStream, see a couple of lines before (because i_pos > tk->i_end before the AStreamRefillStream call). Now, according to the the error check, I assumed that this might be temporarily acceptable, and then, AStreamRefillStream is called to read more data so that i_offset is valid again afterwards. The code looks like it was intended to support that.
> 
> If this is not intended, then indeed, the wrong i_offset should be corrected already before AStreamRefillStream. But in this case, AFAIU i_pos > tk->i_end should never be true in any way, thus the error check might be redundant.

Hello again,

I just tried the alternative proposal, i.e. directly correcting i_offset when it is set to the out of range, with this code before the AStreamRefillStream call:

if (tk->i_end < tk->i_start + p_sys->stream.i_offset) {
            p_sys->stream.i_offset = tk->i_end - tk->i_start;
            // return VLC_EGENERIC;
}

This leads to immediate playback problems (or stop of playback). So indeed, it looks like my assumption is correct that it is possible to have a  temporarily invalid i_offset value, and AStreamRefillStream will refill the stream so that i_offset gets valid again.

For the second chunk, you are right that this is not the best way, and I’ll resend a patch avoiding the casts to int.

With best regards,
David


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20140610/1f47623a/attachment.html>