diff mbox series

[FFmpeg-devel,2/3] avformat/mxfdec: Fix memleak when parsing tag fails

Message ID 20200720061545.18854-2-andreas.rheinhardt@gmail.com
State Accepted
Commit 78f21cab188a094d42520bcad9686c3b5afa844b
Headers show
Series [FFmpeg-devel,1/3] avformat/mxfdec: Fix memleak when adding element to array fails
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Andreas Rheinhardt July 20, 2020, 6:15 a.m. UTC
The MXF demuxer uses an array of pointers to different structures of
metadata (all containing a common initial sequence containing a type
field to distinguish them) and some of these structures contain pointers
to separately allocated subelements. If an error happens while reading
and creating the tags, the semi-finished new tag is freed using the
function to free these tags. But this function doesn't free the already
allocated subelements, because the type has not been set yet. This commit
changes this.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/mxfdec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Tomas Härdin July 22, 2020, 12:39 p.m. UTC | #1
mån 2020-07-20 klockan 08:15 +0200 skrev Andreas Rheinhardt:
> --- a/libavformat/mxfdec.c
> +++ b/libavformat/mxfdec.c
> @@ -2714,6 +2714,7 @@ static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
>  
>  static int mxf_metadataset_init(MXFMetadataSet *ctx, enum MXFMetadataSetType type)
>  {
> +    ctx->type = type;
>      switch (type){
>      case MultipleDescriptor:
>      case Descriptor:
> @@ -2734,7 +2735,8 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF
>  
>      if (!ctx)
>          return AVERROR(ENOMEM);
> -    mxf_metadataset_init(ctx, type);
> +    if (ctx_size)
> +        mxf_metadataset_init(ctx, type);
>      while (avio_tell(pb) + 4 < klv_end && !avio_feof(pb)) {
>          int ret;
>          int tag = avio_rb16(pb);
> @@ -2770,7 +2772,6 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF
>           * it extending past the end of the KLV though (zzuf5.mxf). */
>          if (avio_tell(pb) > klv_end) {
>              if (ctx_size) {
> -                ctx->type = type;
>                  mxf_free_metadataset(&ctx, 1);
>              }
>  
> @@ -2781,7 +2782,6 @@ static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF
>          } else if (avio_tell(pb) <= next)   /* only seek forward, else this can loop for a long time */
>              avio_seek(pb, next, SEEK_SET);
>      }
> -    if (ctx_size) ctx->type = type;

Looks OK as far as I can tell. It's been a while since I dug into
mxfdec's type system.

/Tomas
diff mbox series

Patch

diff --git a/libavformat/mxfdec.c b/libavformat/mxfdec.c
index 08ad92cc0c..3016885e75 100644
--- a/libavformat/mxfdec.c
+++ b/libavformat/mxfdec.c
@@ -2714,6 +2714,7 @@  static const MXFMetadataReadTableEntry mxf_metadata_read_table[] = {
 
 static int mxf_metadataset_init(MXFMetadataSet *ctx, enum MXFMetadataSetType type)
 {
+    ctx->type = type;
     switch (type){
     case MultipleDescriptor:
     case Descriptor:
@@ -2734,7 +2735,8 @@  static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF
 
     if (!ctx)
         return AVERROR(ENOMEM);
-    mxf_metadataset_init(ctx, type);
+    if (ctx_size)
+        mxf_metadataset_init(ctx, type);
     while (avio_tell(pb) + 4 < klv_end && !avio_feof(pb)) {
         int ret;
         int tag = avio_rb16(pb);
@@ -2770,7 +2772,6 @@  static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF
          * it extending past the end of the KLV though (zzuf5.mxf). */
         if (avio_tell(pb) > klv_end) {
             if (ctx_size) {
-                ctx->type = type;
                 mxf_free_metadataset(&ctx, 1);
             }
 
@@ -2781,7 +2782,6 @@  static int mxf_read_local_tags(MXFContext *mxf, KLVPacket *klv, MXFMetadataReadF
         } else if (avio_tell(pb) <= next)   /* only seek forward, else this can loop for a long time */
             avio_seek(pb, next, SEEK_SET);
     }
-    if (ctx_size) ctx->type = type;
     return ctx_size ? mxf_add_metadata_set(mxf, &ctx) : 0;
 }