[vlc-devel] [PATCH] demux/aiff: fix integer overflow leading to infinite loop

[Filip Roséen]

Working on a new set of patches that heavily refactors the
implementation, please view this thread (and the patches inside) as
obsolete.

On 2016-10-29 00:21, Filip Roséen wrote:

> Hi,
> 
> Seems like today is the day where I make a lot of typos and silly
> mistakes.. right after sending the patch I realized that the
> bitfiddling is of course wrong.
> 
> I deeply apologize, and perhaps I could mention that I got news of
> illness recently in the family - so my mind is somewhat scattered at
> the moment.
> 
> -----------------------------------------------------------------------
> 
> In order to align something to a boundrary of 2, `(x + 1) & ~1` does
> the right thing, `( x + 1 ) ^ 1` does not make any sense whatsoever.
> 
> See attached patch for a correct implementation, and the below for a
> difference between the previous submitted, and the new one.
> 
>     -        ssize_t i_chunk_size = ( i_data_size + 8 + 1 ) ^ 1;
>     +        ssize_t i_chunk_size = ( i_data_size + 8 + 1 ) & ~1;
> 
> Best regards,\
> Filip
> 
> -----------------------------------------------------------------------
> 
> On 2016-10-29 00:10, Filip Roséen wrote:
> 
> > The previous implementation could potentially overflow the uint32_t
> > holding the data-size when adding the size of the mandatory header, in
> > order to consume the entire chunk. If that happened we could end up in
> > an infinite loop (given that we are not guaranteed to make progress).
> > 
> > These changes fixes the issue by introducing another variable that is
> > only used for the purpose of storing the chunk (total) size.
> > 
> > fixes #17562
> > 

> From b61e3f77031cd090dd46841bfa9b8244dcfb0d4d Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Filip=20Ros=C3=A9en?= <filip at atch.se>
> Date: Sat, 29 Oct 2016 00:04:23 +0200
> Subject: [PATCH] demux/aiff: fix integer overflow leading to infinite loop
> 
> The previous implementation could potentially overflow the uin32_t
> holding the data-size when adding the size of the mandatory header (in
> order to consume the entire chunk), and if that happened we could end
> up in an infinite loop (given that we are not making progress).
> 
> These changes fixes the issue by introducing another variable that is
> only used for the purpose of storing the chunk (total) size.
> 
> fixes #17562
> 
> --
> 
> I will clean-up the implementation shortly, but thought I would
> address this immidiate issue as soon as possible.
> 
> The demuxer can be made a lot simpler, and a lot safer.
> ---
>  modules/demux/aiff.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/modules/demux/aiff.c b/modules/demux/aiff.c
> index bdd0308..9631fdb 100644
> --- a/modules/demux/aiff.c
> +++ b/modules/demux/aiff.c
> @@ -123,14 +123,13 @@ static int Open( vlc_object_t *p_this )
>  
>      for( ;; )
>      {
> -        uint32_t i_size;
> -
>          if( vlc_stream_Peek( p_demux->s, &p_peek, 8 ) < 8 )
>              goto error;
>  
> -        i_size = GetDWBE( &p_peek[4] );
> +        uint32_t i_data_size = GetDWBE( &p_peek[4] );
> +        ssize_t i_chunk_size = ( i_data_size + 8 + 1 ) & ~1;
>  
> -        msg_Dbg( p_demux, "chunk fcc=%4.4s size=%d", p_peek, i_size );
> +        msg_Dbg( p_demux, "chunk fcc=%4.4s size=%" PRIu32, p_peek, i_data_size );
>  
>          if( !memcmp( p_peek, "COMM", 4 ) )
>          {
> @@ -152,7 +151,7 @@ static int Open( vlc_object_t *p_this )
>                  goto error;
>  
>              p_sys->i_ssnd_pos = vlc_stream_Tell( p_demux->s );
> -            p_sys->i_ssnd_size = i_size;
> +            p_sys->i_ssnd_size = i_data_size;
>              p_sys->i_ssnd_offset = GetDWBE( &p_peek[8] );
>              p_sys->i_ssnd_blocksize = GetDWBE( &p_peek[12] );
>  
> @@ -165,11 +164,8 @@ static int Open( vlc_object_t *p_this )
>              break;
>          }
>  
> -        /* Skip this chunk */
> -        i_size += 8;
> -        if( (i_size % 2) != 0 )
> -            i_size++;
> -        if( vlc_stream_Read( p_demux->s, NULL, i_size ) != (int)i_size )
> +        /* consume chunk */
> +        if( vlc_stream_Read( p_demux->s, NULL, i_chunk_size ) != i_chunk_size )
>          {
>              msg_Warn( p_demux, "incomplete file" );
>              goto error;
> -- 
> 2.10.1
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161029/6b1e461a/attachment.html>