| Message ID | 20191213002810.6440-5-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
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
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 [...]
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
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 [...]
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 --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; }
No testcase Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/hevc_mp4toannexb_bsf.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)