[vlc-devel] [PATCH] demux/asf: fix 17580: prevent overflow leading to crash

Filip Roséen filip at atch.se
Tue Nov 1 14:05:15 CET 2016


Hi

On 2016-11-01 14:56, Rémi Denis-Courmont wrote:

> Le tiistaina 1. marraskuuta 2016, 2.20.26 EET Filip Roséen a écrit :
> > Given that the previous implementation assigned the return-value of
> > vlc_stream_Peek to a size_t, the value would wrap around on error
> > (since vlc_stream_Peek returns -1), rendering the "< 78" somewhat
> > useless (when an error occurs).
> > 
> > These changes change the type of i_peek to correspond to that of
> > vlc_stream_Peek, while also making sure that we error before calling
> > the function if the object size is larger than SSIZE_MAX (meaning that
> > we cannot peek).
> > 
> > fixes #17580
> > ---
> >  modules/demux/asf/libasf.c | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/modules/demux/asf/libasf.c b/modules/demux/asf/libasf.c
> > index ed782bd..b303ce5 100644
> > --- a/modules/demux/asf/libasf.c
> > +++ b/modules/demux/asf/libasf.c
> > @@ -25,6 +25,8 @@
> >  # include "config.h"
> >  #endif
> > 
> > +#include <limits.h>
> > +
> >  #include <vlc_demux.h>
> >  #include <vlc_charset.h>          /* FromCharset */
> > 
> > @@ -528,9 +530,17 @@ static void ASF_FreeObject_header_extension(
> > asf_object_t *p_obj ) static int ASF_ReadObject_stream_properties( stream_t
> > *s, asf_object_t *p_obj ) {
> >      asf_object_stream_properties_t *p_sp = &p_obj->stream_properties;
> > -    size_t        i_peek;
> > +    ssize_t i_peek;
> >      const uint8_t *p_peek;
> > 
> > +#if UINT64_MAX > SSIZE_MAX
> > +    if( p_sp->i_object_size > SSIZE_MAX )
> > +    {
> > +        msg_Err( s, "unable to peek: object size is larger than SSIZE_MAX"
> > );
> 
> I wouldn´t name a constant in debug messages. Either format the value, or just 
> write a generic too large.

See attached patch for one where the diagnostic is fixed.

> 
> > +        return VLC_EGENERIC;
> > +    }
> > +#endif
> > +
> >      if( ( i_peek = vlc_stream_Peek( s, &p_peek,  p_sp->i_object_size ) ) <
> > 78 ) return VLC_EGENERIC;
> 
> (OT rant: I don´t like assignment as predicate where unnecessary.)

I wanted to respect the coding-style of the previous implementation,
so I did not change that line (by intention). I agree with you that
assignment inside a predicate is not very nice (more OT: unless you
are writing C++ where a variable can be declared in the predicate).

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161101/7a31d180/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-demux-asf-fix-17580-prevent-overflow-leading-to-cras.patch
Type: text/x-diff
Size: 1760 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20161101/7a31d180/attachment.patch>


More information about the vlc-devel mailing list