diff mbox

[FFmpeg-devel] avcodec: Avoid splitting side data repeatedly

Message ID 20170504151518.9230-1-michael@niedermayer.cc
State Accepted
Headers show

Commit Message

Michael Niedermayer May 4, 2017, 3:15 p.m. UTC 
Fixes Timeout
Fixes: 508/clusterfuzz-testcase-6245747678773248

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
 libavcodec/decode.c   |  9 +++++++--
 libavcodec/internal.h |  4 ++++
 3 files changed, 35 insertions(+), 2 deletions(-)

Comments

James Almer May 4, 2017, 3:41 p.m. UTC | #1 
On 5/4/2017 12:15 PM, Michael Niedermayer wrote:
> Fixes Timeout
> Fixes: 508/clusterfuzz-testcase-6245747678773248
> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
>  libavcodec/decode.c   |  9 +++++++--
>  libavcodec/internal.h |  4 ++++
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index d97400eaef..de12a1f1ce 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -463,6 +463,30 @@ int av_packet_split_side_data(AVPacket *pkt){
>      return 0;
>  }
>  
> +int ff_packet_split_and_drop_side_data(AVPacket *pkt){
> +    if (!pkt->side_data_elems && pkt->size >12 && AV_RB64(pkt->data + pkt->size - 8) == FF_MERGE_MARKER){
> +        int i;
> +        unsigned int size;
> +        uint8_t *p;
> +
> +        p = pkt->data + pkt->size - 8 - 5;
> +        for (i=1; ; i++){
> +            size = AV_RB32(p);
> +            if (size>INT_MAX - 5 || p - pkt->data < size)
> +                return 0;
> +            if (p[4]&128)
> +                break;
> +            if (p - pkt->data < size + 5)
> +                return 0;
> +            p-= size+5;
> +        }
> +        pkt->size = p - pkt->data - size;
> +        av_assert0(pkt->size >= 0);
> +        return 1;
> +    }
> +    return 0;
> +}
> +
>  #endif
>  
>  uint8_t *av_packet_pack_dictionary(AVDictionary *dict, int *size)
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 708071fd07..43ba04550a 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -392,7 +392,9 @@ static int decode_simple_internal(AVCodecContext *avctx, AVFrame *frame)
>      tmp = *pkt;
>  #if FF_API_MERGE_SD
>  FF_DISABLE_DEPRECATION_WARNINGS
> -    did_split = av_packet_split_side_data(&tmp);
> +    did_split = avci->compat_decode_partial_size ?
> +                ff_packet_split_and_drop_side_data(&tmp) :
> +                av_packet_split_side_data(&tmp);

Eh, this is all going away as soon as we bump major and will not make it
to any upcoming release, so guess it's fine for now.

>  
>      if (did_split) {
>          ret = extract_packet_props(avctx->internal, &tmp);
> @@ -961,6 +963,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
>                               AVPacket *avpkt)
>  {
>      int i, ret = 0;
> +    AVCodecInternal *avci = avctx->internal;
>  
>      if (!avpkt->data && avpkt->size) {
>          av_log(avctx, AV_LOG_ERROR, "invalid packet: NULL data, size != 0\n");
> @@ -981,7 +984,9 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
>          AVPacket tmp = *avpkt;
>  #if FF_API_MERGE_SD
>  FF_DISABLE_DEPRECATION_WARNINGS
> -        int did_split = av_packet_split_side_data(&tmp);
> +        int did_split = avci->compat_decode_partial_size ?
> +                        ff_packet_split_and_drop_side_data(&tmp) :
> +                        av_packet_split_side_data(&tmp);
>          //apply_param_change(avctx, &tmp);
>  
>          if (did_split) {
> diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> index e30d4aa73d..f3d10a14f1 100644
> --- a/libavcodec/internal.h
> +++ b/libavcodec/internal.h
> @@ -356,6 +356,10 @@ int ff_set_sar(AVCodecContext *avctx, AVRational sar);
>  int ff_side_data_update_matrix_encoding(AVFrame *frame,
>                                          enum AVMatrixEncoding matrix_encoding);
>  
> +#if FF_API_MERGE_SD_API
> +int ff_packet_split_and_drop_side_data(AVPacket *pkt);
> +#endif

This function will not be used by anything after the ABI part of this
functionality is dropped, so maybe make it depend on FF_API_MERGE_SD
instead, here and in avpacket.c

> +
>  /**
>   * Select the (possibly hardware accelerated) pixel format.
>   * This is a wrapper around AVCodecContext.get_format() and should be used
>
Michael Niedermayer May 6, 2017, 1:14 a.m. UTC | #2 
On Thu, May 04, 2017 at 12:41:49PM -0300, James Almer wrote:
> On 5/4/2017 12:15 PM, Michael Niedermayer wrote:
> > Fixes Timeout
> > Fixes: 508/clusterfuzz-testcase-6245747678773248
> > 
> > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/targets/ffmpeg
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/avpacket.c | 24 ++++++++++++++++++++++++
> >  libavcodec/decode.c   |  9 +++++++--
> >  libavcodec/internal.h |  4 ++++
> >  3 files changed, 35 insertions(+), 2 deletions(-)
> > 
> > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> > index d97400eaef..de12a1f1ce 100644
> > --- a/libavcodec/avpacket.c
> > +++ b/libavcodec/avpacket.c
> > @@ -463,6 +463,30 @@ int av_packet_split_side_data(AVPacket *pkt){
> >      return 0;
> >  }
> >  
> > +int ff_packet_split_and_drop_side_data(AVPacket *pkt){
> > +    if (!pkt->side_data_elems && pkt->size >12 && AV_RB64(pkt->data + pkt->size - 8) == FF_MERGE_MARKER){
> > +        int i;
> > +        unsigned int size;
> > +        uint8_t *p;
> > +
> > +        p = pkt->data + pkt->size - 8 - 5;
> > +        for (i=1; ; i++){
> > +            size = AV_RB32(p);
> > +            if (size>INT_MAX - 5 || p - pkt->data < size)
> > +                return 0;
> > +            if (p[4]&128)
> > +                break;
> > +            if (p - pkt->data < size + 5)
> > +                return 0;
> > +            p-= size+5;
> > +        }
> > +        pkt->size = p - pkt->data - size;
> > +        av_assert0(pkt->size >= 0);
> > +        return 1;
> > +    }
> > +    return 0;
> > +}
> > +
> >  #endif
> >  
> >  uint8_t *av_packet_pack_dictionary(AVDictionary *dict, int *size)
> > diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> > index 708071fd07..43ba04550a 100644
> > --- a/libavcodec/decode.c
> > +++ b/libavcodec/decode.c
> > @@ -392,7 +392,9 @@ static int decode_simple_internal(AVCodecContext *avctx, AVFrame *frame)
> >      tmp = *pkt;
> >  #if FF_API_MERGE_SD
> >  FF_DISABLE_DEPRECATION_WARNINGS
> > -    did_split = av_packet_split_side_data(&tmp);
> > +    did_split = avci->compat_decode_partial_size ?
> > +                ff_packet_split_and_drop_side_data(&tmp) :
> > +                av_packet_split_side_data(&tmp);
> 
> Eh, this is all going away as soon as we bump major and will not make it
> to any upcoming release, so guess it's fine for now.
> 
> >  
> >      if (did_split) {
> >          ret = extract_packet_props(avctx->internal, &tmp);
> > @@ -961,6 +963,7 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
> >                               AVPacket *avpkt)
> >  {
> >      int i, ret = 0;
> > +    AVCodecInternal *avci = avctx->internal;
> >  
> >      if (!avpkt->data && avpkt->size) {
> >          av_log(avctx, AV_LOG_ERROR, "invalid packet: NULL data, size != 0\n");
> > @@ -981,7 +984,9 @@ int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
> >          AVPacket tmp = *avpkt;
> >  #if FF_API_MERGE_SD
> >  FF_DISABLE_DEPRECATION_WARNINGS
> > -        int did_split = av_packet_split_side_data(&tmp);
> > +        int did_split = avci->compat_decode_partial_size ?
> > +                        ff_packet_split_and_drop_side_data(&tmp) :
> > +                        av_packet_split_side_data(&tmp);
> >          //apply_param_change(avctx, &tmp);
> >  
> >          if (did_split) {
> > diff --git a/libavcodec/internal.h b/libavcodec/internal.h
> > index e30d4aa73d..f3d10a14f1 100644
> > --- a/libavcodec/internal.h
> > +++ b/libavcodec/internal.h
> > @@ -356,6 +356,10 @@ int ff_set_sar(AVCodecContext *avctx, AVRational sar);
> >  int ff_side_data_update_matrix_encoding(AVFrame *frame,
> >                                          enum AVMatrixEncoding matrix_encoding);
> >  
> > +#if FF_API_MERGE_SD_API
> > +int ff_packet_split_and_drop_side_data(AVPacket *pkt);
> > +#endif
> 
> This function will not be used by anything after the ABI part of this
> functionality is dropped, so maybe make it depend on FF_API_MERGE_SD
> instead, here and in avpacket.c

ok, will apply with these changes

thx

[...]
diff mbox

Patch

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index d97400eaef..de12a1f1ce 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -463,6 +463,30 @@  int av_packet_split_side_data(AVPacket *pkt){
     return 0;
 }
 
+int ff_packet_split_and_drop_side_data(AVPacket *pkt){
+    if (!pkt->side_data_elems && pkt->size >12 && AV_RB64(pkt->data + pkt->size - 8) == FF_MERGE_MARKER){
+        int i;
+        unsigned int size;
+        uint8_t *p;
+
+        p = pkt->data + pkt->size - 8 - 5;
+        for (i=1; ; i++){
+            size = AV_RB32(p);
+            if (size>INT_MAX - 5 || p - pkt->data < size)
+                return 0;
+            if (p[4]&128)
+                break;
+            if (p - pkt->data < size + 5)
+                return 0;
+            p-= size+5;
+        }
+        pkt->size = p - pkt->data - size;
+        av_assert0(pkt->size >= 0);
+        return 1;
+    }
+    return 0;
+}
+
 #endif
 
 uint8_t *av_packet_pack_dictionary(AVDictionary *dict, int *size)
diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 708071fd07..43ba04550a 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -392,7 +392,9 @@  static int decode_simple_internal(AVCodecContext *avctx, AVFrame *frame)
     tmp = *pkt;
 #if FF_API_MERGE_SD
 FF_DISABLE_DEPRECATION_WARNINGS
-    did_split = av_packet_split_side_data(&tmp);
+    did_split = avci->compat_decode_partial_size ?
+                ff_packet_split_and_drop_side_data(&tmp) :
+                av_packet_split_side_data(&tmp);
 
     if (did_split) {
         ret = extract_packet_props(avctx->internal, &tmp);
@@ -961,6 +963,7 @@  int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
                              AVPacket *avpkt)
 {
     int i, ret = 0;
+    AVCodecInternal *avci = avctx->internal;
 
     if (!avpkt->data && avpkt->size) {
         av_log(avctx, AV_LOG_ERROR, "invalid packet: NULL data, size != 0\n");
@@ -981,7 +984,9 @@  int avcodec_decode_subtitle2(AVCodecContext *avctx, AVSubtitle *sub,
         AVPacket tmp = *avpkt;
 #if FF_API_MERGE_SD
 FF_DISABLE_DEPRECATION_WARNINGS
-        int did_split = av_packet_split_side_data(&tmp);
+        int did_split = avci->compat_decode_partial_size ?
+                        ff_packet_split_and_drop_side_data(&tmp) :
+                        av_packet_split_side_data(&tmp);
         //apply_param_change(avctx, &tmp);
 
         if (did_split) {
diff --git a/libavcodec/internal.h b/libavcodec/internal.h
index e30d4aa73d..f3d10a14f1 100644
--- a/libavcodec/internal.h
+++ b/libavcodec/internal.h
@@ -356,6 +356,10 @@  int ff_set_sar(AVCodecContext *avctx, AVRational sar);
 int ff_side_data_update_matrix_encoding(AVFrame *frame,
                                         enum AVMatrixEncoding matrix_encoding);
 
+#if FF_API_MERGE_SD_API
+int ff_packet_split_and_drop_side_data(AVPacket *pkt);
+#endif
+
 /**
  * Select the (possibly hardware accelerated) pixel format.
  * This is a wrapper around AVCodecContext.get_format() and should be used