[FFmpeg-devel] [PATCH 1/2] avformat/mpc8: fix broken pointer math

wm4 nfxjfg at googlemail.com
Wed Feb 4 13:18:54 CET 2015


On Wed, 4 Feb 2015 01:00:06 +0100
Michael Niedermayer <michaelni at gmx.at> wrote:

> On Tue, Feb 03, 2015 at 09:44:13PM +0100, Reimar Döffinger wrote:
> > On Tue, Feb 03, 2015 at 07:04:11PM +0100, wm4 wrote:
> > > This could overflow and crash at least on 32 bit systems.
> > > ---
> > >  libavformat/mpc8.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/libavformat/mpc8.c b/libavformat/mpc8.c
> > > index a15dc25..d6ca338 100644
> > > --- a/libavformat/mpc8.c
> > > +++ b/libavformat/mpc8.c
> > > @@ -91,7 +91,7 @@ static int mpc8_probe(AVProbeData *p)
> > >          size = bs_get_v(&bs);
> > >          if (size < 2)
> > >              return 0;
> > > -        if (bs + size - 2 >= bs_end)
> > > +        if (size >= bs_end - bs + 2)
> > >              return AVPROBE_SCORE_EXTENSION - 1; // seems to be valid MPC but no header yet
> > 
> > Seems ok to me, but for consistency/ease of checking
> 
> applied
> 
> 
> > I'd suggest changing while (bs < bs_end + 3) to this style
> > as well.
> > However there is one more issue:
> > bs_get_v can overflow.
> > And as the compiler can assume that signed overflow
> > will not happen, that case is not certain to be
> > caught by the size < 2 check, and thus these cases
> > might escape all checks.
> > Stupid question: Why do we support size values of whole
> > 64 bits?
> > Nobody has invented any storage media where one could store that much.
> > Changing the limit in bs_get_v from 10 to e.g. 6 or 7 should
> > avoid the issue...
> 
> i would suggest:
> 
> @@ -57,7 +57,7 @@ typedef struct {
> 
>  static inline int64_t bs_get_v(const uint8_t **bs)
>  {
> -    int64_t v = 0;
> +    uint64_t v = 0;
>      int br = 0;
>      int c;

Probably a good idea. I think shifting into the sign is always
undefined behavior?

> 
> thanks
> 
> [...]



More information about the ffmpeg-devel mailing list