diff mbox series

[FFmpeg-devel,1/2] lavc/aac_ac3_parser: fix the potential overflow

Message ID 1593436991-20076-1-git-send-email-mypopydev@gmail.com
State New
Headers show
Series [FFmpeg-devel,1/2] lavc/aac_ac3_parser: fix the potential overflow
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Jun Zhao June 29, 2020, 1:23 p.m. UTC
From: Jun Zhao <barryjzhao@tencent.com>

Fix the potential overflow.

Suggested-by: Alexander Strasser <eclipse7@gmx.net>
Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
---
 libavcodec/aac_ac3_parser.c         | 9 +++++----
 libavcodec/aac_ac3_parser.h         | 4 ++--
 tests/ref/fate/adtstoasc_ticket3715 | 2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

Comments

Anton Khirnov July 1, 2020, 2:23 p.m. UTC | #1
Quoting Jun Zhao (2020-06-29 15:23:10)
> From: Jun Zhao <barryjzhao@tencent.com>
> 
> Fix the potential overflow.
> 
> Suggested-by: Alexander Strasser <eclipse7@gmx.net>
> Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> ---
>  libavcodec/aac_ac3_parser.c         | 9 +++++----
>  libavcodec/aac_ac3_parser.h         | 4 ++--
>  tests/ref/fate/adtstoasc_ticket3715 | 2 +-
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
> index 0746798..b26790d 100644
> --- a/libavcodec/aac_ac3_parser.c
> +++ b/libavcodec/aac_ac3_parser.c
> @@ -98,11 +98,12 @@ get_next:
>          }
>  
>          /* Calculate the average bit rate */
> -        s->frame_number++;
>          if (avctx->codec_id != AV_CODEC_ID_EAC3) {
> -            avctx->bit_rate =
> -                (s->last_bit_rate * (s->frame_number -1) + s->bit_rate)/s->frame_number;
> -            s->last_bit_rate = avctx->bit_rate;
> +            if (s->frame_number < UINT64_MAX) {
> +                s->frame_number++;
> +                s->last_bit_rate += (s->bit_rate - s->last_bit_rate)/s->frame_number;
> +                avctx->bit_rate = (int64_t)llround(s->last_bit_rate);
> +            }
>          }
>      }
>  
> diff --git a/libavcodec/aac_ac3_parser.h b/libavcodec/aac_ac3_parser.h
> index b04041f..c53d16f 100644
> --- a/libavcodec/aac_ac3_parser.h
> +++ b/libavcodec/aac_ac3_parser.h
> @@ -55,8 +55,8 @@ typedef struct AACAC3ParseContext {
>      uint64_t state;
>  
>      int need_next_header;
> -    int frame_number;
> -    int last_bit_rate;
> +    uint64_t frame_number;
> +    double last_bit_rate;

This won't give the same result on all platforms anymore.
Alexander Strasser July 1, 2020, 7:05 p.m. UTC | #2
Hi Anton,
hi Jun!

On 2020-07-01 16:23 +0200, Anton Khirnov wrote:
> Quoting Jun Zhao (2020-06-29 15:23:10)
> > From: Jun Zhao <barryjzhao@tencent.com>
> >
> > Fix the potential overflow.
> >
> > Suggested-by: Alexander Strasser <eclipse7@gmx.net>
> > Signed-off-by: Jun Zhao <barryjzhao@tencent.com>
> > ---
> >  libavcodec/aac_ac3_parser.c         | 9 +++++----
> >  libavcodec/aac_ac3_parser.h         | 4 ++--
> >  tests/ref/fate/adtstoasc_ticket3715 | 2 +-
> >  3 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
> > index 0746798..b26790d 100644
> > --- a/libavcodec/aac_ac3_parser.c
> > +++ b/libavcodec/aac_ac3_parser.c
> > @@ -98,11 +98,12 @@ get_next:
> >          }
> >
> >          /* Calculate the average bit rate */
> > -        s->frame_number++;
> >          if (avctx->codec_id != AV_CODEC_ID_EAC3) {
> > -            avctx->bit_rate =
> > -                (s->last_bit_rate * (s->frame_number -1) + s->bit_rate)/s->frame_number;
> > -            s->last_bit_rate = avctx->bit_rate;
> > +            if (s->frame_number < UINT64_MAX) {
> > +                s->frame_number++;
> > +                s->last_bit_rate += (s->bit_rate - s->last_bit_rate)/s->frame_number;
> > +                avctx->bit_rate = (int64_t)llround(s->last_bit_rate);
> > +            }
> >          }
> >      }
> >
> > diff --git a/libavcodec/aac_ac3_parser.h b/libavcodec/aac_ac3_parser.h
> > index b04041f..c53d16f 100644
> > --- a/libavcodec/aac_ac3_parser.h
> > +++ b/libavcodec/aac_ac3_parser.h
> > @@ -55,8 +55,8 @@ typedef struct AACAC3ParseContext {
> >      uint64_t state;
> >
> >      int need_next_header;
> > -    int frame_number;
> > -    int last_bit_rate;
> > +    uint64_t frame_number;
> > +    double last_bit_rate;
>
> This won't give the same result on all platforms anymore.

It's also a bit different from what I had in mind.

I was thinking more in the line of how it's implemented in
libavcodec/mpegaudio_parser.c .

There is a bit of noise there because of data that doesn't contain audio
and also for the CBR case I think. Wouldn't be needed here AFAICT.

I may well be missing something. If so understanding more would help me
and we could fix both places. Otherwise if it's OK like it was done in
mpegaudio_parser, we could maybe use the same strategy here too.


Thanks for sending the patch and sorry for the delayed response.


Best regards,
  Alexander
diff mbox series

Patch

diff --git a/libavcodec/aac_ac3_parser.c b/libavcodec/aac_ac3_parser.c
index 0746798..b26790d 100644
--- a/libavcodec/aac_ac3_parser.c
+++ b/libavcodec/aac_ac3_parser.c
@@ -98,11 +98,12 @@  get_next:
         }
 
         /* Calculate the average bit rate */
-        s->frame_number++;
         if (avctx->codec_id != AV_CODEC_ID_EAC3) {
-            avctx->bit_rate =
-                (s->last_bit_rate * (s->frame_number -1) + s->bit_rate)/s->frame_number;
-            s->last_bit_rate = avctx->bit_rate;
+            if (s->frame_number < UINT64_MAX) {
+                s->frame_number++;
+                s->last_bit_rate += (s->bit_rate - s->last_bit_rate)/s->frame_number;
+                avctx->bit_rate = (int64_t)llround(s->last_bit_rate);
+            }
         }
     }
 
diff --git a/libavcodec/aac_ac3_parser.h b/libavcodec/aac_ac3_parser.h
index b04041f..c53d16f 100644
--- a/libavcodec/aac_ac3_parser.h
+++ b/libavcodec/aac_ac3_parser.h
@@ -55,8 +55,8 @@  typedef struct AACAC3ParseContext {
     uint64_t state;
 
     int need_next_header;
-    int frame_number;
-    int last_bit_rate;
+    uint64_t frame_number;
+    double last_bit_rate;
     enum AVCodecID codec_id;
 } AACAC3ParseContext;
 
diff --git a/tests/ref/fate/adtstoasc_ticket3715 b/tests/ref/fate/adtstoasc_ticket3715
index 3b473ee..63e577a 100644
--- a/tests/ref/fate/adtstoasc_ticket3715
+++ b/tests/ref/fate/adtstoasc_ticket3715
@@ -1,4 +1,4 @@ 
-3e63cbb6bb6ec756d79fab2632fef305 *tests/data/fate/adtstoasc_ticket3715.mov
+feb8a43e4b7df2a4763bd9d55153c900 *tests/data/fate/adtstoasc_ticket3715.mov
 33324 tests/data/fate/adtstoasc_ticket3715.mov
 #extradata 0:        2, 0x00340022
 #tb 0: 1/44100