diff mbox

[FFmpeg-devel,5/5] avcodec/hevc_mp4toannexb_bsf: Check that there is enough input left for nalu size

Message ID 20191213002810.6440-5-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer Dec. 13, 2019, 12:28 a.m. UTC
No testcase

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/hevc_mp4toannexb_bsf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Andreas Rheinhardt Dec. 13, 2019, 11:24 a.m. UTC | #1
On Fri, Dec 13, 2019 at 3:07 AM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> No testcase
>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/hevc_mp4toannexb_bsf.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> b/libavcodec/hevc_mp4toannexb_bsf.c
> index baa93628ed..e0d20a550c 100644
> --- a/libavcodec/hevc_mp4toannexb_bsf.c
> +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> @@ -152,7 +152,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx,
> AVPacket *out)
>          extra_size    = add_extradata * ctx->par_out->extradata_size;
>          got_irap     |= is_irap;
>
> -        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size) {
> +        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size ||
>

Up until now I thought that FFmpeg has some implicit assumptions: int
having 32bit being one of them (the log2 functions depend on this). And I
also thought that size_t being able to hold all the values of an unsigned
was one of these implicit assumptions, too. Am I wrong on this?

A testcase for the last condition is easy to produce by simply manipulating
the size field of a NAL unit.

- Andreas
Michael Niedermayer Dec. 13, 2019, 4:56 p.m. UTC | #2
On Fri, Dec 13, 2019 at 12:24:04PM +0100, Andreas Rheinhardt wrote:
> On Fri, Dec 13, 2019 at 3:07 AM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > No testcase
> >
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/hevc_mp4toannexb_bsf.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> > b/libavcodec/hevc_mp4toannexb_bsf.c
> > index baa93628ed..e0d20a550c 100644
> > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > @@ -152,7 +152,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext *ctx,
> > AVPacket *out)
> >          extra_size    = add_extradata * ctx->par_out->extradata_size;
> >          got_irap     |= is_irap;
> >
> > -        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size) {
> > +        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size ||
> >
> 
> Up until now I thought that FFmpeg has some implicit assumptions: int
> having 32bit being one of them (the log2 functions depend on this). And I

yes, that was from POSIX


> also thought that size_t being able to hold all the values of an unsigned
> was one of these implicit assumptions, too. Am I wrong on this?

I was asking myself the same, and i couldnt really find anything where we
stated that previously so i added a FFMIN.


> 
> A testcase for the last condition is easy to produce by simply manipulating
> the size field of a NAL unit.

yes, do you think we should create such a testcase for this fix ?

thx

[...]
Andreas Rheinhardt Dec. 13, 2019, 5:05 p.m. UTC | #3
On Fri, Dec 13, 2019 at 5:56 PM Michael Niedermayer <michael@niedermayer.cc>
wrote:

> On Fri, Dec 13, 2019 at 12:24:04PM +0100, Andreas Rheinhardt wrote:
> > On Fri, Dec 13, 2019 at 3:07 AM Michael Niedermayer
> <michael@niedermayer.cc>
> > wrote:
> >
> > > No testcase
> > >
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/hevc_mp4toannexb_bsf.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> > > b/libavcodec/hevc_mp4toannexb_bsf.c
> > > index baa93628ed..e0d20a550c 100644
> > > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > > @@ -152,7 +152,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext
> *ctx,
> > > AVPacket *out)
> > >          extra_size    = add_extradata * ctx->par_out->extradata_size;
> > >          got_irap     |= is_irap;
> > >
> > > -        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size)
> {
> > > +        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size
> ||
> > >
> >
> > Up until now I thought that FFmpeg has some implicit assumptions: int
> > having 32bit being one of them (the log2 functions depend on this). And I
>
> yes, that was from POSIX
>
>
> > also thought that size_t being able to hold all the values of an unsigned
> > was one of these implicit assumptions, too. Am I wrong on this?
>
> I was asking myself the same, and i couldnt really find anything where we
> stated that previously so i added a FFMIN.
>
> In this case we would have to add lots of checks before av_malloc and
other allocation functions that use size_t parameters in order to ensure
that no loss of information happens when an int (or unsigned) is converted
to size_t. In other words: We should not support such systems.

>
> >
> > A testcase for the last condition is easy to produce by simply
> manipulating
> > the size field of a NAL unit.
>
> yes, do you think we should create such a testcase for this fix ?
>

You mean a fate test? Why not?

- Andreas
Michael Niedermayer Dec. 13, 2019, 8 p.m. UTC | #4
On Fri, Dec 13, 2019 at 06:05:21PM +0100, Andreas Rheinhardt wrote:
> On Fri, Dec 13, 2019 at 5:56 PM Michael Niedermayer <michael@niedermayer.cc>
> wrote:
> 
> > On Fri, Dec 13, 2019 at 12:24:04PM +0100, Andreas Rheinhardt wrote:
> > > On Fri, Dec 13, 2019 at 3:07 AM Michael Niedermayer
> > <michael@niedermayer.cc>
> > > wrote:
> > >
> > > > No testcase
> > > >
> > > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > > ---
> > > >  libavcodec/hevc_mp4toannexb_bsf.c | 4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
> > > > b/libavcodec/hevc_mp4toannexb_bsf.c
> > > > index baa93628ed..e0d20a550c 100644
> > > > --- a/libavcodec/hevc_mp4toannexb_bsf.c
> > > > +++ b/libavcodec/hevc_mp4toannexb_bsf.c
> > > > @@ -152,7 +152,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext
> > *ctx,
> > > > AVPacket *out)
> > > >          extra_size    = add_extradata * ctx->par_out->extradata_size;
> > > >          got_irap     |= is_irap;
> > > >
> > > > -        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size)
> > {
> > > > +        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size
> > ||
> > > >
> > >
> > > Up until now I thought that FFmpeg has some implicit assumptions: int
> > > having 32bit being one of them (the log2 functions depend on this). And I
> >
> > yes, that was from POSIX
> >
> >
> > > also thought that size_t being able to hold all the values of an unsigned
> > > was one of these implicit assumptions, too. Am I wrong on this?
> >
> > I was asking myself the same, and i couldnt really find anything where we
> > stated that previously so i added a FFMIN.
> >
> > In this case we would have to add lots of checks before av_malloc and
> other allocation functions that use size_t parameters in order to ensure
> that no loss of information happens when an int (or unsigned) is converted
> to size_t. In other words: We should not support such systems.

If we would choose to support such platforms in the future then using our own
type thats the bigger of the 2 might make this relativly painless. But first
such a platform needs to be relevant for us and exist ...
And if we choose to assume things about these types, that should be in our
developer documentation.

But to return to the patch here, do you want me to remove the FFMIN ?

thanks

[...]
Andreas Rheinhardt Dec. 19, 2019, 6:50 p.m. UTC | #5
Michael Niedermayer:
> On Fri, Dec 13, 2019 at 06:05:21PM +0100, Andreas Rheinhardt wrote:
>> On Fri, Dec 13, 2019 at 5:56 PM Michael Niedermayer <michael@niedermayer.cc>
>> wrote:
>>
>>> On Fri, Dec 13, 2019 at 12:24:04PM +0100, Andreas Rheinhardt wrote:
>>>> On Fri, Dec 13, 2019 at 3:07 AM Michael Niedermayer
>>> <michael@niedermayer.cc>
>>>> wrote:
>>>>
>>>>> No testcase
>>>>>
>>>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>>>> ---
>>>>>  libavcodec/hevc_mp4toannexb_bsf.c | 4 +++-
>>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/libavcodec/hevc_mp4toannexb_bsf.c
>>>>> b/libavcodec/hevc_mp4toannexb_bsf.c
>>>>> index baa93628ed..e0d20a550c 100644
>>>>> --- a/libavcodec/hevc_mp4toannexb_bsf.c
>>>>> +++ b/libavcodec/hevc_mp4toannexb_bsf.c
>>>>> @@ -152,7 +152,9 @@ static int hevc_mp4toannexb_filter(AVBSFContext
>>> *ctx,
>>>>> AVPacket *out)
>>>>>          extra_size    = add_extradata * ctx->par_out->extradata_size;
>>>>>          got_irap     |= is_irap;
>>>>>
>>>>> -        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size)
>>> {
>>>>> +        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size
>>> ||
>>>>>
>>>>
>>>> Up until now I thought that FFmpeg has some implicit assumptions: int
>>>> having 32bit being one of them (the log2 functions depend on this). And I
>>>
>>> yes, that was from POSIX
>>>
>>>
>>>> also thought that size_t being able to hold all the values of an unsigned
>>>> was one of these implicit assumptions, too. Am I wrong on this?
>>>
>>> I was asking myself the same, and i couldnt really find anything where we
>>> stated that previously so i added a FFMIN.
>>>
>>> In this case we would have to add lots of checks before av_malloc and
>> other allocation functions that use size_t parameters in order to ensure
>> that no loss of information happens when an int (or unsigned) is converted
>> to size_t. In other words: We should not support such systems.
> 
> If we would choose to support such platforms in the future then using our own
> type thats the bigger of the 2 might make this relativly painless. But first
> such a platform needs to be relevant for us and exist ...
> And if we choose to assume things about these types, that should be in our
> developer documentation.
> 
> But to return to the patch here, do you want me to remove the FFMIN ?
> 

Yes, please do so. After all, as long as there is no platform relevant
to us it is just an unnecessary complication/potential for confusion.

- Andreas
diff mbox

Patch

diff --git a/libavcodec/hevc_mp4toannexb_bsf.c b/libavcodec/hevc_mp4toannexb_bsf.c
index baa93628ed..e0d20a550c 100644
--- a/libavcodec/hevc_mp4toannexb_bsf.c
+++ b/libavcodec/hevc_mp4toannexb_bsf.c
@@ -152,7 +152,9 @@  static int hevc_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *out)
         extra_size    = add_extradata * ctx->par_out->extradata_size;
         got_irap     |= is_irap;
 
-        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size) {
+        if (FFMIN(INT_MAX, SIZE_MAX) < 4ULL + nalu_size + extra_size ||
+            bytestream2_get_bytes_left(&gb) < nalu_size
+        ) {
             ret = AVERROR_INVALIDDATA;
             goto fail;
         }