[FFmpeg-devel,1/6] avcodec/h264_slice: Check input SPS in ff_h264_update_thread_context()

Fixes: crash
Fixes: check_pkt.mp4

Found-by: Rafael Dutra <rafael.dutra@cispa.de>
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/h264_slice.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

On Mon, Mar 22, 2021 at 10:05 PM Michael Niedermayer
<michael@niedermayer.cc> wrote:
>
> Fixes: crash
> Fixes: check_pkt.mp4
>
> Found-by: Rafael Dutra <rafael.dutra@cispa.de>
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/h264_slice.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 14b945756b..910d8b8848 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -304,9 +304,8 @@ int ff_h264_update_thread_context(AVCodecContext *dst,
>      if (dst == src)
>          return 0;
>
> -    // We can't fail if SPS isn't set at it breaks current skip_frame code
> -    //if (!h1->ps.sps)
> -    //    return AVERROR_INVALIDDATA;
> +    if (inited && !h1->ps.sps)
> +        return AVERROR_INVALIDDATA;
>

Any considerations for the removed comment?

- Hendrik

On Mon, Mar 22, 2021 at 10:12:47PM +0100, Hendrik Leppkes wrote:
> On Mon, Mar 22, 2021 at 10:05 PM Michael Niedermayer
> <michael@niedermayer.cc> wrote:
> >
> > Fixes: crash
> > Fixes: check_pkt.mp4
> >
> > Found-by: Rafael Dutra <rafael.dutra@cispa.de>
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/h264_slice.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> > index 14b945756b..910d8b8848 100644
> > --- a/libavcodec/h264_slice.c
> > +++ b/libavcodec/h264_slice.c
> > @@ -304,9 +304,8 @@ int ff_h264_update_thread_context(AVCodecContext *dst,
> >      if (dst == src)
> >          return 0;
> >
> > -    // We can't fail if SPS isn't set at it breaks current skip_frame code
> > -    //if (!h1->ps.sps)
> > -    //    return AVERROR_INVALIDDATA;
> > +    if (inited && !h1->ps.sps)
> > +        return AVERROR_INVALIDDATA;
> >
> 
> Any considerations for the removed comment?

Without the inited, i could reproduce a skip_frame failure, replicating what
i assumed the comment meant.

With inited, the next lines would determine if we need to reinit but 
without h1->ps.sps the current set of checks have a good chance of
dereferencing a null pointer. So this looks like a clean failure is
better. And unlikely that was occuring in any use case.

So i think the comment, at least as written is not correct.

Thanks

[...]

On Tue, Mar 23, 2021 at 12:34:16AM +0100, Michael Niedermayer wrote:
> On Mon, Mar 22, 2021 at 10:12:47PM +0100, Hendrik Leppkes wrote:
> > On Mon, Mar 22, 2021 at 10:05 PM Michael Niedermayer
> > <michael@niedermayer.cc> wrote:
> > >
> > > Fixes: crash
> > > Fixes: check_pkt.mp4
> > >
> > > Found-by: Rafael Dutra <rafael.dutra@cispa.de>
> > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > > ---
> > >  libavcodec/h264_slice.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> > > index 14b945756b..910d8b8848 100644
> > > --- a/libavcodec/h264_slice.c
> > > +++ b/libavcodec/h264_slice.c
> > > @@ -304,9 +304,8 @@ int ff_h264_update_thread_context(AVCodecContext *dst,
> > >      if (dst == src)
> > >          return 0;
> > >
> > > -    // We can't fail if SPS isn't set at it breaks current skip_frame code
> > > -    //if (!h1->ps.sps)
> > > -    //    return AVERROR_INVALIDDATA;
> > > +    if (inited && !h1->ps.sps)
> > > +        return AVERROR_INVALIDDATA;
> > >
> > 
> > Any considerations for the removed comment?
> 
> Without the inited, i could reproduce a skip_frame failure, replicating what
> i assumed the comment meant.
> 
> With inited, the next lines would determine if we need to reinit but 
> without h1->ps.sps the current set of checks have a good chance of
> dereferencing a null pointer. So this looks like a clean failure is
> better. And unlikely that was occuring in any use case.
> 
> So i think the comment, at least as written is not correct.

a week passed with no comments, so i intend to apply this as it fixes a
null pointer dereference

thx

[...]