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

[David Fuhrmann]

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.

With best regards,
David

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20140609/2c98cc8d/attachment.html>