[FFmpeg-devel] Fix signed integer overflow undefined behavior

Submitted by Nikolas Bowe on Jan. 19, 2018, 10:48 p.m.

Details

Message ID 20180119224808.5055-1-nbowe@google.com
State New
Headers show

Commit Message

Nikolas Bowe Jan. 19, 2018, 10:48 p.m.
Found via fuzzing
---
 libavformat/rpl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer Jan. 19, 2018, 11 p.m.
On Fri, Jan 19, 2018 at 02:48:08PM -0800, Nikolas Bowe wrote:
> Found via fuzzing
> ---
>  libavformat/rpl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/libavformat/rpl.c b/libavformat/rpl.c
> index d373600478..df449bfc29 100644
> --- a/libavformat/rpl.c
> +++ b/libavformat/rpl.c
> @@ -194,7 +194,7 @@ static int rpl_read_header(AVFormatContext *s)
>          if (ast->codecpar->bits_per_coded_sample == 0)
>              ast->codecpar->bits_per_coded_sample = 4;
>  
> -        ast->codecpar->bit_rate = ast->codecpar->sample_rate *
> +        ast->codecpar->bit_rate = (uint64_t)ast->codecpar->sample_rate *
>                                    ast->codecpar->bits_per_coded_sample *
>                                    ast->codecpar->channels;

uint64_t is the wrong type, bit_rate is int64_t

[...]
Nikolas Bowe Jan. 19, 2018, 11:56 p.m.
On Fri, Jan 19, 2018 at 3:00 PM, Michael Niedermayer <michael@niedermayer.cc
> wrote:

> On Fri, Jan 19, 2018 at 02:48:08PM -0800, Nikolas Bowe wrote:
> > Found via fuzzing
> > ---
> >  libavformat/rpl.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/libavformat/rpl.c b/libavformat/rpl.c
> > index d373600478..df449bfc29 100644
> > --- a/libavformat/rpl.c
> > +++ b/libavformat/rpl.c
> > @@ -194,7 +194,7 @@ static int rpl_read_header(AVFormatContext *s)
> >          if (ast->codecpar->bits_per_coded_sample == 0)
> >              ast->codecpar->bits_per_coded_sample = 4;
> >
> > -        ast->codecpar->bit_rate = ast->codecpar->sample_rate *
> > +        ast->codecpar->bit_rate = (uint64_t)ast->codecpar->sample_rate
> *
> >                                    ast->codecpar->bits_per_coded_sample
> *
> >                                    ast->codecpar->channels;
>
> uint64_t is the wrong type, bit_rate is int64_t
>
>
That is true, but wouldn't fix the overflow
signed int64_t can still overflow eg if sample_rate, bits_per_coded_sample,
and channels are all INT_MAX.
By using uint64_t, the overflow is at least defined behavior.

It might be better to range check these values, but I could not find a
comprehensive spec.
I could use reasonably lenient values of say 384 khz and 32 bits per coded
sample.
Would you prefer that?



> [...]
> --
> Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB
>
> I know you won't believe me, but the highest form of Human Excellence is
> to question oneself and others. -- Socrates
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
>
Michael Niedermayer Jan. 20, 2018, 12:46 a.m.
On Fri, Jan 19, 2018 at 03:56:19PM -0800, Niki Bowe wrote:
> On Fri, Jan 19, 2018 at 3:00 PM, Michael Niedermayer <michael@niedermayer.cc
> > wrote:
> 
> > On Fri, Jan 19, 2018 at 02:48:08PM -0800, Nikolas Bowe wrote:
> > > Found via fuzzing
> > > ---
> > >  libavformat/rpl.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/libavformat/rpl.c b/libavformat/rpl.c
> > > index d373600478..df449bfc29 100644
> > > --- a/libavformat/rpl.c
> > > +++ b/libavformat/rpl.c
> > > @@ -194,7 +194,7 @@ static int rpl_read_header(AVFormatContext *s)
> > >          if (ast->codecpar->bits_per_coded_sample == 0)
> > >              ast->codecpar->bits_per_coded_sample = 4;
> > >
> > > -        ast->codecpar->bit_rate = ast->codecpar->sample_rate *
> > > +        ast->codecpar->bit_rate = (uint64_t)ast->codecpar->sample_rate
> > *
> > >                                    ast->codecpar->bits_per_coded_sample
> > *
> > >                                    ast->codecpar->channels;
> >
> > uint64_t is the wrong type, bit_rate is int64_t
> >
> >
> That is true, but wouldn't fix the overflow
> signed int64_t can still overflow eg if sample_rate, bits_per_coded_sample,
> and channels are all INT_MAX.
> By using uint64_t, the overflow is at least defined behavior.
> 
> It might be better to range check these values, but I could not find a
> comprehensive spec.
> I could use reasonably lenient values of say 384 khz and 32 bits per coded
> sample.
> Would you prefer that?

correct range checks would be ideal. if that is not possible then at least
bit_rate should not be set "randomly" in case of an overflow


thx

[...]

Patch hide | download patch | download mbox

diff --git a/libavformat/rpl.c b/libavformat/rpl.c
index d373600478..df449bfc29 100644
--- a/libavformat/rpl.c
+++ b/libavformat/rpl.c
@@ -194,7 +194,7 @@  static int rpl_read_header(AVFormatContext *s)
         if (ast->codecpar->bits_per_coded_sample == 0)
             ast->codecpar->bits_per_coded_sample = 4;
 
-        ast->codecpar->bit_rate = ast->codecpar->sample_rate *
+        ast->codecpar->bit_rate = (uint64_t)ast->codecpar->sample_rate *
                                   ast->codecpar->bits_per_coded_sample *
                                   ast->codecpar->channels;