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

[Rémi Denis-Courmont]

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.

>              return VLC_EGENERIC;
> +        }
>      }
>      return VLC_SUCCESS;
>  }
> @@ -1339,8 +1341,8 @@ static int AStreamRefillStream( stream_t *s )
> 
>      /* We read but won't increase i_start after initial start + offset */
>      int i_toread =
> -        __MIN( p_sys->stream.i_used, STREAM_CACHE_TRACK_SIZE -
> -               (tk->i_end - tk->i_start - p_sys->stream.i_offset) );
> +        __MIN( (int)p_sys->stream.i_used, STREAM_CACHE_TRACK_SIZE -
> +               ((int64_t)tk->i_end - (int64_t)tk->i_start -
> (int)p_sys->stream.i_offset) );

The conversion can trigger undefined signed integer overflow, I think.

>      bool b_read = false;
>      int64_t i_start, i_stop;

-- 
Rémi Denis-Courmont
http://www.remlab.net/