diff mbox series

[FFmpeg-devel,2/4] avformat/mxfdec: Check count in mxf_read_strong_ref_array()

Message ID 20220312235227.19626-2-michael@niedermayer.cc
State Accepted
Commit 3015c556f316d4ab364ed55e8bc97cc0f2cc57a3
Headers
Series [FFmpeg-devel,1/4] avcodec/vp9_superframe_split_bsf: Check in size |

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Michael Niedermayer March 12, 2022, 11:52 p.m. UTC 
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavformat/mxfdec.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Tomas Härdin March 14, 2022, 7:19 p.m. UTC | #1 
sön 2022-03-13 klockan 00:52 +0100 skrev Michael Niedermayer:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavformat/mxfdec.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> index b85c10bf19..d7cdd22c8a 100644
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -932,7 +932,13 @@ static int mxf_read_cryptographic_context(void
> *arg, AVIOContext *pb, int tag, i
>  
>  static int mxf_read_strong_ref_array(AVIOContext *pb, UID **refs,
> int *count)
>  {
> -    *count = avio_rb32(pb);
> +    unsigned c = avio_rb32(pb);

not uint32_t?

> +
> +    //avio_read() used int
> +    if (c > INT_MAX / sizeof(UID))
> +        return AVERROR_PATCHWELCOME;
> +    *count = c;
> +

This should already be caught by av_calloc(), no?

/Tomas
Michael Niedermayer March 19, 2022, 10:50 p.m. UTC | #2 
On Mon, Mar 14, 2022 at 08:19:51PM +0100, Tomas Härdin wrote:
> sön 2022-03-13 klockan 00:52 +0100 skrev Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavformat/mxfdec.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > index b85c10bf19..d7cdd22c8a 100644
> > --- a/libavformat/mxfdec.c
> > +++ b/libavformat/mxfdec.c
> > @@ -932,7 +932,13 @@ static int mxf_read_cryptographic_context(void
> > *arg, AVIOContext *pb, int tag, i
> >  
> >  static int mxf_read_strong_ref_array(AVIOContext *pb, UID **refs,
> > int *count)
> >  {
> > -    *count = avio_rb32(pb);
> > +    unsigned c = avio_rb32(pb);
> 
> not uint32_t?

we dont need an exact length variable here


> 
> > +
> > +    //avio_read() used int
> > +    if (c > INT_MAX / sizeof(UID))
> > +        return AVERROR_PATCHWELCOME;
> > +    *count = c;
> > +
> 
> This should already be caught by av_calloc(), no?

the API as in the documentation of av_calloc() does not gurantee
this. Its bad practice if we write code that depends on some implementation
of some code in a diferent module/lib

thx

[...]
Tomas Härdin March 20, 2022, 1:05 p.m. UTC | #3 
lör 2022-03-19 klockan 23:50 +0100 skrev Michael Niedermayer:
> On Mon, Mar 14, 2022 at 08:19:51PM +0100, Tomas Härdin wrote:
> > sön 2022-03-13 klockan 00:52 +0100 skrev Michael Niedermayer:
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavformat/mxfdec.c | 8 +++++++-
> > >  1 file changed, 7 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
> > > index b85c10bf19..d7cdd22c8a 100644
> > > --- a/libavformat/mxfdec.c
> > > +++ b/libavformat/mxfdec.c
> > > @@ -932,7 +932,13 @@ static int
> > > mxf_read_cryptographic_context(void
> > > *arg, AVIOContext *pb, int tag, i
> > >  
> > >  static int mxf_read_strong_ref_array(AVIOContext *pb, UID
> > > **refs,
> > > int *count)
> > >  {
> > > -    *count = avio_rb32(pb);
> > > +    unsigned c = avio_rb32(pb);
> > 
> > not uint32_t?
> 
> we dont need an exact length variable here

Right, we don't support 16-bit machines

> 
> 
> > 
> > > +
> > > +    //avio_read() used int
> > > +    if (c > INT_MAX / sizeof(UID))
> > > +        return AVERROR_PATCHWELCOME;
> > > +    *count = c;
> > > +
> > 
> > This should already be caught by av_calloc(), no?
> 
> the API as in the documentation of av_calloc() does not gurantee
> this. 

Yes it does:

  The allocated memory will have size `size * nmemb` bytes.
  [...]
  `NULL` if the block cannot be allocated

> Its bad practice if we write code that depends on some implementation
> of some code in a diferent module/lib

If av_calloc() does not guarantee this then it is useless. It is used
precisely for this all over the place. Are you going to change every
use of av_calloc() in mxfdec in the same way?

/Tomas
Michael Niedermayer March 20, 2022, 2:06 p.m. UTC | #4 
On Sun, Mar 20, 2022 at 02:05:41PM +0100, Tomas Härdin wrote:
> lör 2022-03-19 klockan 23:50 +0100 skrev Michael Niedermayer:
[...]
> > 
> > 
> > > 
> > > > +
> > > > +    //avio_read() used int
> > > > +    if (c > INT_MAX / sizeof(UID))
> > > > +        return AVERROR_PATCHWELCOME;
> > > > +    *count = c;
> > > > +
> > > 
> > > This should already be caught by av_calloc(), no?
> > 
> > the API as in the documentation of av_calloc() does not gurantee
> > this. 
> 
> Yes it does:
> 
>   The allocated memory will have size `size * nmemb` bytes.
>   [...]
>   `NULL` if the block cannot be allocated

void *av_calloc(size_t nmemb, size_t size)
size_t can be larger than int, so size * nmemb may be larger than INT_MAX


> 
> > Its bad practice if we write code that depends on some implementation
> > of some code in a diferent module/lib
> 
> If av_calloc() does not guarantee this then it is useless. It is used
> precisely for this all over the place. Are you going to change every
> use of av_calloc() in mxfdec in the same way?

well, when max_alloc_size is set above INT_MAX 
then int checks will become needed when these values ever get stored in
ints. For example here avio_read() has a int argument that is set to the
product of the 2. Or all such ints need to be changed to something bigger

thx

[...]
Tomas Härdin March 21, 2022, 10:06 a.m. UTC | #5 
sön 2022-03-20 klockan 15:06 +0100 skrev Michael Niedermayer:
> On Sun, Mar 20, 2022 at 02:05:41PM +0100, Tomas Härdin wrote:
> > lör 2022-03-19 klockan 23:50 +0100 skrev Michael Niedermayer:
> [...]
> > > 
> > > 
> > > > 
> > > > > +
> > > > > +    //avio_read() used int
> > > > > +    if (c > INT_MAX / sizeof(UID))
> > > > > +        return AVERROR_PATCHWELCOME;
> > > > > +    *count = c;
> > > > > +
> > > > 
> > > > This should already be caught by av_calloc(), no?
> > > 
> > > the API as in the documentation of av_calloc() does not gurantee
> > > this. 
> > 
> > Yes it does:
> > 
> >   The allocated memory will have size `size * nmemb` bytes.
> >   [...]
> >   `NULL` if the block cannot be allocated
> 
> void *av_calloc(size_t nmemb, size_t size)
> size_t can be larger than int, so size * nmemb may be larger than
> INT_MAX

Crap, you're right. This also brings to mind the question why
packages_count etc are int rather than unsigned or uint32_t..

Patch is OK then

/Tomas
Michael Niedermayer March 21, 2022, 9:20 p.m. UTC | #6 
On Mon, Mar 21, 2022 at 11:06:14AM +0100, Tomas Härdin wrote:
> sön 2022-03-20 klockan 15:06 +0100 skrev Michael Niedermayer:
> > On Sun, Mar 20, 2022 at 02:05:41PM +0100, Tomas Härdin wrote:
> > > lör 2022-03-19 klockan 23:50 +0100 skrev Michael Niedermayer:
> > [...]
> > > > 
> > > > 
> > > > > 
> > > > > > +
> > > > > > +    //avio_read() used int
> > > > > > +    if (c > INT_MAX / sizeof(UID))
> > > > > > +        return AVERROR_PATCHWELCOME;
> > > > > > +    *count = c;
> > > > > > +
> > > > > 
> > > > > This should already be caught by av_calloc(), no?
> > > > 
> > > > the API as in the documentation of av_calloc() does not gurantee
> > > > this. 
> > > 
> > > Yes it does:
> > > 
> > >   The allocated memory will have size `size * nmemb` bytes.
> > >   [...]
> > >   `NULL` if the block cannot be allocated
> > 
> > void *av_calloc(size_t nmemb, size_t size)
> > size_t can be larger than int, so size * nmemb may be larger than
> > INT_MAX
> 
> Crap, you're right. This also brings to mind the question why
> packages_count etc are int rather than unsigned or uint32_t..
> 
> Patch is OK then

will apply

thx

[...]
diff mbox series

Patch

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index b85c10bf19..d7cdd22c8a 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -932,7 +932,13 @@  static int mxf_read_cryptographic_context(void *arg, AVIOContext *pb, int tag, i
 
 static int mxf_read_strong_ref_array(AVIOContext *pb, UID **refs, int *count)
 {
-    *count = avio_rb32(pb);
+    unsigned c = avio_rb32(pb);
+
+    //avio_read() used int
+    if (c > INT_MAX / sizeof(UID))
+        return AVERROR_PATCHWELCOME;
+    *count = c;
+
     av_free(*refs);
     *refs = av_calloc(*count, sizeof(UID));
     if (!*refs) {