| Message ID | 20220312235227.19626-2-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 3015c556f316d4ab364ed55e8bc97cc0f2cc57a3 |
| Headers | show |
| Series | [FFmpeg-devel,1/4] avcodec/vp9_superframe_split_bsf: Check in size | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
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
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 [...]
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
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 [...]
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
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 --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) {
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavformat/mxfdec.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)