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

2019-12-19 Thread Andreas Rheinhardt 
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 
>> 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
>>> 
 wrote:

> No testcase
>
> Signed-off-by: Michael Niedermayer 
> ---
>  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2019-12-13 Thread 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 
> 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
> > 
> > > wrote:
> > >
> > > > No testcase
> > > >
> > > > Signed-off-by: Michael Niedermayer 
> > > > ---
> > > >  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 GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Breaking DRM is a little like attempting to break through a door even
though the window is wide open and the only thing in the house is a bunch
of things you dont want and which you would get tomorrow for free anyway


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2019-12-13 Thread Andreas Rheinhardt 
On Fri, Dec 13, 2019 at 5:56 PM Michael Niedermayer 
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
> 
> > wrote:
> >
> > > No testcase
> > >
> > > Signed-off-by: Michael Niedermayer 
> > > ---
> > >  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2019-12-13 Thread Michael Niedermayer 
On Fri, Dec 13, 2019 at 12:24:04PM +0100, Andreas Rheinhardt wrote:
> On Fri, Dec 13, 2019 at 3:07 AM Michael Niedermayer 
> wrote:
> 
> > No testcase
> >
> > Signed-off-by: Michael Niedermayer 
> > ---
> >  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

[...]
-- 
Michael GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The real ebay dictionary, page 1
"Used only once"- "Some unspecified defect prevented a second use"
"In good condition" - "Can be repaird by experienced expert"
"As is" - "You wouldnt want it even if you were payed for it, if you knew ..."


signature.asc
Description: PGP signature
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2019-12-13 Thread Andreas Rheinhardt 
On Fri, Dec 13, 2019 at 3:07 AM Michael Niedermayer 
wrote:

> No testcase
>
> Signed-off-by: Michael Niedermayer 
> ---
>  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
___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".

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

2019-12-12 Thread Michael Niedermayer 
No testcase

Signed-off-by: Michael Niedermayer 
---
 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 ||
+bytestream2_get_bytes_left() < nalu_size
+) {
 ret = AVERROR_INVALIDDATA;
 goto fail;
 }
-- 
2.24.0

___
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".