diff mbox series

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

Message ID 20210322205833.14541-1-michael@niedermayer.cc
State Accepted
Commit ceae92cb291c2536a93482cdf3c1ae3f7330b924
Headers show
Series [FFmpeg-devel,1/6] avcodec/h264_slice: Check input SPS in ff_h264_update_thread_context()
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Michael Niedermayer March 22, 2021, 8:58 p.m. UTC
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(-)

Comments

Hendrik Leppkes March 22, 2021, 9:12 p.m. UTC | #1
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
Michael Niedermayer March 22, 2021, 11:34 p.m. UTC | #2
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

[...]
Michael Niedermayer March 31, 2021, 6:42 p.m. UTC | #3
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

[...]
diff mbox series

Patch

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;
 
     if (inited &&
         (h->width                 != h1->width                 ||