diff mbox series

[FFmpeg-devel,7/8] avformat/aviobuf: Check for overflow in ffio_read_varlen()

Message ID 20201218232208.14207-7-michael@niedermayer.cc
State New
Headers show
Series [FFmpeg-devel,1/8] avformat/mpegts: Increase pcr_incr width to 64bit | expand

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished

Commit Message

Michael Niedermayer Dec. 18, 2020, 11:22 p.m. UTC 
No testcase

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/aviobuf.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Andreas Rheinhardt Dec. 21, 2020, 11:30 a.m. UTC | #1 
Michael Niedermayer:
> No testcase
> 
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/aviobuf.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> index 78cc60b2ae..7730547106 100644
> --- a/libavformat/aviobuf.c
> +++ b/libavformat/aviobuf.c
> @@ -917,6 +917,8 @@ uint64_t ffio_read_varlen(AVIOContext *bc){
>  
>      do{
>          tmp = avio_r8(bc);
> +        if (val > UINT64_MAX>>7)
> +            return AVERROR_INVALIDDATA;
>          val= (val<<7) + (tmp&127);
>      }while(tmp&128);
>      return val;
> 
The error can't be detected at all given that the function returns an
uint64_t.

- Andreas
Michael Niedermayer Dec. 21, 2020, 5:56 p.m. UTC | #2 
On Mon, Dec 21, 2020 at 12:30:48PM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > No testcase
> > 
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/aviobuf.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
> > index 78cc60b2ae..7730547106 100644
> > --- a/libavformat/aviobuf.c
> > +++ b/libavformat/aviobuf.c
> > @@ -917,6 +917,8 @@ uint64_t ffio_read_varlen(AVIOContext *bc){
> >  
> >      do{
> >          tmp = avio_r8(bc);
> > +        if (val > UINT64_MAX>>7)
> > +            return AVERROR_INVALIDDATA;
> >          val= (val<<7) + (tmp&127);
> >      }while(tmp&128);
> >      return val;
> > 
> The error can't be detected at all given that the function returns an
> uint64_t.

in practice the type of the destination matters.
But sure you are correct that this is not "great"
what do you suggest ? there are many ways to fix this
also the reuse of this function between demuxers feels not entirely
wise. Any change/fix must be reasonable for all demuxers, i imagine
that makes the mainaince for all harder ...

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/aviobuf.c b/libavformat/aviobuf.c
index 78cc60b2ae..7730547106 100644
--- a/libavformat/aviobuf.c
+++ b/libavformat/aviobuf.c
@@ -917,6 +917,8 @@  uint64_t ffio_read_varlen(AVIOContext *bc){
 
     do{
         tmp = avio_r8(bc);
+        if (val > UINT64_MAX>>7)
+            return AVERROR_INVALIDDATA;
         val= (val<<7) + (tmp&127);
     }while(tmp&128);
     return val;