[FFmpeg-devel] [PATCH 1/5] avformat/iamf_writer: Don't leak on error when adding ParamDefinition

[FFmpeg-devel] [PATCH 1/5] avformat/iamf_writer: Don't leak on error when adding ParamDefinition

[Andreas Rheinhardt]

Fix this by postponing the allocation.
Fixes Coverity issue #1559545.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/iamf_writer.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
index 1a360dee2f..897b06310f 100644
--- a/libavformat/iamf_writer.c
+++ b/libavformat/iamf_writer.c
@@ -138,10 +138,6 @@ static IAMFParamDefinition *add_param_definition(IAMFContext *iamf, AVIAMFParamD
 
     iamf->param_definitions = tmp;
 
-    param_definition = av_mallocz(sizeof(*param_definition));
-    if (!param_definition)
-        return NULL;
-
     if (audio_element)
         codec_config = iamf->codec_configs[audio_element->codec_config_id];
 
@@ -160,6 +156,10 @@ static IAMFParamDefinition *add_param_definition(IAMFContext *iamf, AVIAMFParamD
             param->constant_subblock_duration = codec_config->nb_samples;
     }
 
+    param_definition = av_mallocz(sizeof(*param_definition));
+    if (!param_definition)
+        return NULL;
+
     param_definition->mode = !!param->duration;
     param_definition->param = param;
     param_definition->audio_element = audio_element;
-- 
2.34.1

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

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

[FFmpeg-devel] [PATCH 2/5] avformat/iamf_writer: Remove nonsense check

[Andreas Rheinhardt]

Checking whether a pointer to an element of an array is NULL
makes no sense, as the pointer addition involved in getting
the address would be undefined behaviour already if the array
were NULL.
In this case the array allocation has already been checked
a few lines before.
Fixes Coverity issue #1559548.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/iamf_writer.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
index 897b06310f..e0c47b5e9d 100644
--- a/libavformat/iamf_writer.c
+++ b/libavformat/iamf_writer.c
@@ -251,8 +251,6 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
         int nb_channels = iamf_audio_element->layers[i]->ch_layout.nb_channels;
 
         IAMFLayer *layer = &audio_element->layers[i];
-        if (!layer)
-            return AVERROR(ENOMEM);
         memset(layer, 0, sizeof(*layer));
 
         if (i)
-- 
2.34.1

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

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

Re: [FFmpeg-devel] [PATCH 2/5] avformat/iamf_writer: Remove nonsense check

[James Almer]

On 2/19/2024 6:52 PM, Andreas Rheinhardt wrote:
> Checking whether a pointer to an element of an array is NULL
> makes no sense, as the pointer addition involved in getting
> the address would be undefined behaviour already if the array
> were NULL.
> In this case the array allocation has already been checked
> a few lines before.
> Fixes Coverity issue #1559548.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/iamf_writer.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
> index 897b06310f..e0c47b5e9d 100644
> --- a/libavformat/iamf_writer.c
> +++ b/libavformat/iamf_writer.c
> @@ -251,8 +251,6 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
>           int nb_channels = iamf_audio_element->layers[i]->ch_layout.nb_channels;
>   
>           IAMFLayer *layer = &audio_element->layers[i];
> -        if (!layer)
> -            return AVERROR(ENOMEM);
>           memset(layer, 0, sizeof(*layer));
>   
>           if (i)

LGTM. I noticed this while rewriting the mp4 implementation, but forgot 
to deal with it.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[FFmpeg-devel] [PATCH 3/5] avformat/iamf_writer: Don't memset twice

[Andreas Rheinhardt]

This has been allocated via av_calloc() a few lines above.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/iamf_writer.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
index e0c47b5e9d..9a665dc002 100644
--- a/libavformat/iamf_writer.c
+++ b/libavformat/iamf_writer.c
@@ -251,7 +251,6 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
         int nb_channels = iamf_audio_element->layers[i]->ch_layout.nb_channels;
 
         IAMFLayer *layer = &audio_element->layers[i];
-        memset(layer, 0, sizeof(*layer));
 
         if (i)
             nb_channels -= iamf_audio_element->layers[i - 1]->ch_layout.nb_channels;
-- 
2.34.1

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

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

Re: [FFmpeg-devel] [PATCH 3/5] avformat/iamf_writer: Don't memset twice

[James Almer]

On 2/19/2024 6:52 PM, Andreas Rheinhardt wrote:
> This has been allocated via av_calloc() a few lines above.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/iamf_writer.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
> index e0c47b5e9d..9a665dc002 100644
> --- a/libavformat/iamf_writer.c
> +++ b/libavformat/iamf_writer.c
> @@ -251,7 +251,6 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
>           int nb_channels = iamf_audio_element->layers[i]->ch_layout.nb_channels;
>   
>           IAMFLayer *layer = &audio_element->layers[i];
> -        memset(layer, 0, sizeof(*layer));
>   
>           if (i)
>               nb_channels -= iamf_audio_element->layers[i - 1]->ch_layout.nb_channels;

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

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

[FFmpeg-devel] [PATCH 4/5] avformat/iamf: Don't mix ownership and non-ownership pointers

[Andreas Rheinhardt]

IAMFAudioElement and IAMFMixPresentation currently contain
pointers to independently allocated objects that are sometimes
owned by said structures and sometimes not.

More precisely, upon success the demuxer transfers ownership
of these other objects newly created AVStreamGroups, but it
keeps its pointers. iamf_read_close() therefore always resets
these pointers (because the cleanup code always treats them
as ownership pointers). This leads to memory leaks in case
iamf_read_header() without having attached all of these
objects to stream groups.

The muxer has a similar issue: It also clears these pointers
(pointing to objects owned by stream groups created by the user)
in its deinit function.

This commit fixes this memleak by explicitly adding non-ownership
pointers; this also allows to remove the code to reset the
ownership pointers.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/iamf.h        | 10 ++++++++++
 libavformat/iamf_parse.c  |  2 ++
 libavformat/iamf_writer.c | 16 ++++++++--------
 libavformat/iamfdec.c     | 22 ++++++++--------------
 libavformat/iamfenc.c     | 12 +-----------
 5 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/libavformat/iamf.h b/libavformat/iamf.h
index d88a24c435..0cb0902e86 100644
--- a/libavformat/iamf.h
+++ b/libavformat/iamf.h
@@ -86,6 +86,11 @@ typedef struct IAMFSubStream {
 } IAMFSubStream;
 
 typedef struct IAMFAudioElement {
+    const AVIAMFAudioElement *celement;
+    /**
+     * element backs celement iff the AVIAMFAudioElement
+     * is owned by this structure.
+     */
     AVIAMFAudioElement *element;
     unsigned int audio_element_id;
 
@@ -100,6 +105,11 @@ typedef struct IAMFAudioElement {
 } IAMFAudioElement;
 
 typedef struct IAMFMixPresentation {
+    const AVIAMFMixPresentation *cmix;
+    /**
+     * mix backs cmix iff the AVIAMFMixPresentation
+     * is owned by this structure.
+     */
     AVIAMFMixPresentation *mix;
     unsigned int mix_presentation_id;
 
diff --git a/libavformat/iamf_parse.c b/libavformat/iamf_parse.c
index a6443f4f3d..50dfd1a6c2 100644
--- a/libavformat/iamf_parse.c
+++ b/libavformat/iamf_parse.c
@@ -651,6 +651,7 @@ static int audio_element_obu(void *s, IAMFContext *c, AVIOContext *pb, int len)
         ret = AVERROR(ENOMEM);
         goto fail;
     }
+    audio_element->celement = element;
 
     element->audio_element_type = audio_element_type;
 
@@ -809,6 +810,7 @@ static int mix_presentation_obu(void *s, IAMFContext *c, AVIOContext *pb, int le
         ret = AVERROR(ENOMEM);
         goto fail;
     }
+    mix_presentation->cmix = mix;
 
     mix_presentation->count_label = ffio_read_leb(pbc);
     mix_presentation->language_label = av_calloc(mix_presentation->count_label,
diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
index 9a665dc002..e8a88b44c3 100644
--- a/libavformat/iamf_writer.c
+++ b/libavformat/iamf_writer.c
@@ -234,7 +234,7 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
     if (!audio_element)
         return AVERROR(ENOMEM);
 
-    audio_element->element = stg->params.iamf_audio_element;
+    audio_element->celement = stg->params.iamf_audio_element;
     audio_element->audio_element_id = stg->id;
     audio_element->codec_config_id = ret;
 
@@ -329,11 +329,11 @@ int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, vo
     if (!mix_presentation)
         return AVERROR(ENOMEM);
 
-    mix_presentation->mix = stg->params.iamf_mix_presentation;
+    mix_presentation->cmix = stg->params.iamf_mix_presentation;
     mix_presentation->mix_presentation_id = stg->id;
 
-    for (int i = 0; i < mix_presentation->mix->nb_submixes; i++) {
-        const AVIAMFSubmix *submix = mix_presentation->mix->submixes[i];
+    for (int i = 0; i < mix_presentation->cmix->nb_submixes; i++) {
+        const AVIAMFSubmix *submix = mix_presentation->cmix->submixes[i];
         AVIAMFParamDefinition *param = submix->output_mix_config;
         IAMFParamDefinition *param_definition;
 
@@ -465,7 +465,7 @@ static inline int rescale_rational(AVRational q, int b)
 static int scalable_channel_layout_config(const IAMFAudioElement *audio_element,
                                           AVIOContext *dyn_bc)
 {
-    const AVIAMFAudioElement *element = audio_element->element;
+    const AVIAMFAudioElement *element = audio_element->celement;
     uint8_t header[MAX_IAMF_OBU_HEADER_SIZE];
     PutBitContext pb;
 
@@ -503,7 +503,7 @@ static int scalable_channel_layout_config(const IAMFAudioElement *audio_element,
 static int ambisonics_config(const IAMFAudioElement *audio_element,
                              AVIOContext *dyn_bc)
 {
-    const AVIAMFAudioElement *element = audio_element->element;
+    const AVIAMFAudioElement *element = audio_element->celement;
     AVIAMFLayer *layer = element->layers[0];
 
     ffio_write_leb(dyn_bc, 0); // ambisonics_mode
@@ -565,7 +565,7 @@ static int iamf_write_audio_element(const IAMFContext *iamf,
                                     const IAMFAudioElement *audio_element,
                                     AVIOContext *pb, void *log_ctx)
 {
-    const AVIAMFAudioElement *element = audio_element->element;
+    const AVIAMFAudioElement *element = audio_element->celement;
     const IAMFCodecConfig *codec_config = iamf->codec_configs[audio_element->codec_config_id];
     uint8_t header[MAX_IAMF_OBU_HEADER_SIZE];
     AVIOContext *dyn_bc;
@@ -669,7 +669,7 @@ static int iamf_write_mixing_presentation(const IAMFContext *iamf,
                                           AVIOContext *pb, void *log_ctx)
 {
     uint8_t header[MAX_IAMF_OBU_HEADER_SIZE];
-    const AVIAMFMixPresentation *mix = mix_presentation->mix;
+    const AVIAMFMixPresentation *mix = mix_presentation->cmix;
     const AVDictionaryEntry *tag = NULL;
     PutBitContext pbc;
     AVIOContext *dyn_bc;
diff --git a/libavformat/iamfdec.c b/libavformat/iamfdec.c
index 99622f697b..9b3f4c7429 100644
--- a/libavformat/iamfdec.c
+++ b/libavformat/iamfdec.c
@@ -232,7 +232,7 @@ static int parameter_block_obu(AVFormatContext *s, int len)
         case AV_IAMF_PARAMETER_DEFINITION_RECON_GAIN: {
             AVIAMFReconGain *recon = subblock;
             const IAMFAudioElement *audio_element = param_definition->audio_element;
-            const AVIAMFAudioElement *element = audio_element->element;
+            const AVIAMFAudioElement *element = audio_element->celement;
 
             av_assert0(audio_element && element);
             for (int i = 0; i < element->nb_layers; i++) {
@@ -403,7 +403,9 @@ static int iamf_read_header(AVFormatContext *s)
 
         av_iamf_audio_element_free(&stg->params.iamf_audio_element);
         stg->id = audio_element->audio_element_id;
+        /* Transfer ownership */
         stg->params.iamf_audio_element = audio_element->element;
+        audio_element->element = NULL;
 
         for (int j = 0; j < audio_element->nb_substreams; j++) {
             IAMFSubStream *substream = &audio_element->substreams[j];
@@ -428,20 +430,22 @@ static int iamf_read_header(AVFormatContext *s)
     for (int i = 0; i < iamf->nb_mix_presentations; i++) {
         IAMFMixPresentation *mix_presentation = iamf->mix_presentations[i];
         AVStreamGroup *stg = avformat_stream_group_create(s, AV_STREAM_GROUP_PARAMS_IAMF_MIX_PRESENTATION, NULL);
-        const AVIAMFMixPresentation *mix = mix_presentation->mix;
+        const AVIAMFMixPresentation *mix = mix_presentation->cmix;
 
         if (!stg)
             return AVERROR(ENOMEM);
 
         av_iamf_mix_presentation_free(&stg->params.iamf_mix_presentation);
         stg->id = mix_presentation->mix_presentation_id;
+        /* Transfer ownership */
         stg->params.iamf_mix_presentation = mix_presentation->mix;
+        mix_presentation->mix = NULL;
 
         for (int j = 0; j < mix->nb_submixes; j++) {
-            AVIAMFSubmix *sub_mix = mix->submixes[j];
+            const AVIAMFSubmix *sub_mix = mix->submixes[j];
 
             for (int k = 0; k < sub_mix->nb_elements; k++) {
-                AVIAMFSubmixElement *submix_element = sub_mix->elements[k];
+                const AVIAMFSubmixElement *submix_element = sub_mix->elements[k];
                 AVStreamGroup *audio_element = NULL;
 
                 for (int l = 0; l < s->nb_stream_groups; l++)
@@ -467,16 +471,6 @@ static int iamf_read_header(AVFormatContext *s)
 static int iamf_read_close(AVFormatContext *s)
 {
     IAMFDemuxContext *const c = s->priv_data;
-    IAMFContext *const iamf = &c->iamf;
-
-    for (int i = 0; i < iamf->nb_audio_elements; i++) {
-        IAMFAudioElement *audio_element = iamf->audio_elements[i];
-        audio_element->element = NULL;
-    }
-    for (int i = 0; i < iamf->nb_mix_presentations; i++) {
-        IAMFMixPresentation *mix_presentation = iamf->mix_presentations[i];
-        mix_presentation->mix = NULL;
-    }
 
     ff_iamf_uninit_context(&c->iamf);
 
diff --git a/libavformat/iamfenc.c b/libavformat/iamfenc.c
index b588a507bb..b5572c79af 100644
--- a/libavformat/iamfenc.c
+++ b/libavformat/iamfenc.c
@@ -218,7 +218,7 @@ static int write_parameter_block(AVFormatContext *s, const AVIAMFParamDefinition
         }
         case AV_IAMF_PARAMETER_DEFINITION_RECON_GAIN: {
             const AVIAMFReconGain *recon = subblock;
-            const AVIAMFAudioElement *audio_element = param_definition->audio_element->element;
+            const AVIAMFAudioElement *audio_element = param_definition->audio_element->celement;
 
             if (!param_definition->mode && param->constant_subblock_duration == 0)
                 ffio_write_leb(dyn_bc, recon->subblock_duration);
@@ -353,16 +353,6 @@ static void iamf_deinit(AVFormatContext *s)
     IAMFMuxContext *const c = s->priv_data;
     IAMFContext *const iamf = &c->iamf;
 
-    for (int i = 0; i < iamf->nb_audio_elements; i++) {
-        IAMFAudioElement *audio_element = iamf->audio_elements[i];
-        audio_element->element = NULL;
-    }
-
-    for (int i = 0; i < iamf->nb_mix_presentations; i++) {
-        IAMFMixPresentation *mix_presentation = iamf->mix_presentations[i];
-        mix_presentation->mix = NULL;
-    }
-
     ff_iamf_uninit_context(iamf);
 
     return;
-- 
2.34.1

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

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

Re: [FFmpeg-devel] [PATCH 4/5] avformat/iamf: Don't mix ownership and non-ownership pointers

[James Almer]

On 2/19/2024 6:52 PM, Andreas Rheinhardt wrote:
> IAMFAudioElement and IAMFMixPresentation currently contain
> pointers to independently allocated objects that are sometimes
> owned by said structures and sometimes not.
> 
> More precisely, upon success the demuxer transfers ownership
> of these other objects newly created AVStreamGroups, but it
> keeps its pointers. iamf_read_close() therefore always resets
> these pointers (because the cleanup code always treats them
> as ownership pointers). This leads to memory leaks in case
> iamf_read_header() without having attached all of these
> objects to stream groups.
> 
> The muxer has a similar issue: It also clears these pointers
> (pointing to objects owned by stream groups created by the user)
> in its deinit function.
> 
> This commit fixes this memleak by explicitly adding non-ownership
> pointers; this also allows to remove the code to reset the
> ownership pointers.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/iamf.h        | 10 ++++++++++
>   libavformat/iamf_parse.c  |  2 ++
>   libavformat/iamf_writer.c | 16 ++++++++--------
>   libavformat/iamfdec.c     | 22 ++++++++--------------
>   libavformat/iamfenc.c     | 12 +-----------
>   5 files changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/libavformat/iamf.h b/libavformat/iamf.h
> index d88a24c435..0cb0902e86 100644
> --- a/libavformat/iamf.h
> +++ b/libavformat/iamf.h
> @@ -86,6 +86,11 @@ typedef struct IAMFSubStream {
>   } IAMFSubStream;
>   
>   typedef struct IAMFAudioElement {
> +    const AVIAMFAudioElement *celement;
> +    /**
> +     * element backs celement iff the AVIAMFAudioElement

Typo. Also in IAMFMixPresentation.

Should be ok otherwise.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

Re: [FFmpeg-devel] [PATCH 4/5] avformat/iamf: Don't mix ownership and non-ownership pointers

[Andreas Rheinhardt]

James Almer:
> On 2/19/2024 6:52 PM, Andreas Rheinhardt wrote:
>> IAMFAudioElement and IAMFMixPresentation currently contain
>> pointers to independently allocated objects that are sometimes
>> owned by said structures and sometimes not.
>>
>> More precisely, upon success the demuxer transfers ownership
>> of these other objects newly created AVStreamGroups, but it
>> keeps its pointers. iamf_read_close() therefore always resets
>> these pointers (because the cleanup code always treats them
>> as ownership pointers). This leads to memory leaks in case
>> iamf_read_header() without having attached all of these
>> objects to stream groups.
>>
>> The muxer has a similar issue: It also clears these pointers
>> (pointing to objects owned by stream groups created by the user)
>> in its deinit function.
>>
>> This commit fixes this memleak by explicitly adding non-ownership
>> pointers; this also allows to remove the code to reset the
>> ownership pointers.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavformat/iamf.h        | 10 ++++++++++
>>   libavformat/iamf_parse.c  |  2 ++
>>   libavformat/iamf_writer.c | 16 ++++++++--------
>>   libavformat/iamfdec.c     | 22 ++++++++--------------
>>   libavformat/iamfenc.c     | 12 +-----------
>>   5 files changed, 29 insertions(+), 33 deletions(-)
>>
>> diff --git a/libavformat/iamf.h b/libavformat/iamf.h
>> index d88a24c435..0cb0902e86 100644
>> --- a/libavformat/iamf.h
>> +++ b/libavformat/iamf.h
>> @@ -86,6 +86,11 @@ typedef struct IAMFSubStream {
>>   } IAMFSubStream;
>>     typedef struct IAMFAudioElement {
>> +    const AVIAMFAudioElement *celement;
>> +    /**
>> +     * element backs celement iff the AVIAMFAudioElement
> 
> Typo. Also in IAMFMixPresentation.
> 
> Should be ok otherwise.

What typo? ("iff" is shorthand for "if and only if".)

- 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-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 4/5] avformat/iamf: Don't mix ownership and non-ownership pointers

[James Almer]

On 2/19/2024 7:06 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 2/19/2024 6:52 PM, Andreas Rheinhardt wrote:
>>> IAMFAudioElement and IAMFMixPresentation currently contain
>>> pointers to independently allocated objects that are sometimes
>>> owned by said structures and sometimes not.
>>>
>>> More precisely, upon success the demuxer transfers ownership
>>> of these other objects newly created AVStreamGroups, but it
>>> keeps its pointers. iamf_read_close() therefore always resets
>>> these pointers (because the cleanup code always treats them
>>> as ownership pointers). This leads to memory leaks in case
>>> iamf_read_header() without having attached all of these
>>> objects to stream groups.
>>>
>>> The muxer has a similar issue: It also clears these pointers
>>> (pointing to objects owned by stream groups created by the user)
>>> in its deinit function.
>>>
>>> This commit fixes this memleak by explicitly adding non-ownership
>>> pointers; this also allows to remove the code to reset the
>>> ownership pointers.
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>> ---
>>>    libavformat/iamf.h        | 10 ++++++++++
>>>    libavformat/iamf_parse.c  |  2 ++
>>>    libavformat/iamf_writer.c | 16 ++++++++--------
>>>    libavformat/iamfdec.c     | 22 ++++++++--------------
>>>    libavformat/iamfenc.c     | 12 +-----------
>>>    5 files changed, 29 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/libavformat/iamf.h b/libavformat/iamf.h
>>> index d88a24c435..0cb0902e86 100644
>>> --- a/libavformat/iamf.h
>>> +++ b/libavformat/iamf.h
>>> @@ -86,6 +86,11 @@ typedef struct IAMFSubStream {
>>>    } IAMFSubStream;
>>>      typedef struct IAMFAudioElement {
>>> +    const AVIAMFAudioElement *celement;
>>> +    /**
>>> +     * element backs celement iff the AVIAMFAudioElement
>>
>> Typo. Also in IAMFMixPresentation.
>>
>> Should be ok otherwise.
> 
> What typo? ("iff" is shorthand for "if and only if".)

First time i see it. But no strong feelings about it.

> 
> - 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-request@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[FFmpeg-devel] [PATCH 5/5] avformat/iamf_writer: Fix leaks on error

[Andreas Rheinhardt]

Fixes Coverity issues #1559544 and #1559547.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/iamf_writer.c | 42 +++++++++++++++++++++++++++------------
 1 file changed, 29 insertions(+), 13 deletions(-)

diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
index e8a88b44c3..f6ebb4aaea 100644
--- a/libavformat/iamf_writer.c
+++ b/libavformat/iamf_writer.c
@@ -240,12 +240,12 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
 
     audio_element->substreams = av_calloc(stg->nb_streams, sizeof(*audio_element->substreams));
     if (!audio_element->substreams)
-        return AVERROR(ENOMEM);
+        goto fail_enomem;
     audio_element->nb_substreams = stg->nb_streams;
 
     audio_element->layers = av_calloc(iamf_audio_element->nb_layers, sizeof(*audio_element->layers));
     if (!audio_element->layers)
-        return AVERROR(ENOMEM);
+        goto fail_enomem;
 
     for (int i = 0, j = 0; i < iamf_audio_element->nb_layers; i++) {
         int nb_channels = iamf_audio_element->layers[i]->ch_layout.nb_channels;
@@ -266,7 +266,8 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
         if (nb_channels) {
             av_log(log_ctx, AV_LOG_ERROR, "Invalid channel count across substreams in layer %u from stream group %u\n",
                    i, stg->index);
-            return AVERROR(EINVAL);
+            ret = AVERROR(EINVAL);
+            goto fail;
         }
     }
 
@@ -276,13 +277,14 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
 
         if (param->nb_subblocks != 1) {
             av_log(log_ctx, AV_LOG_ERROR, "nb_subblocks in demixing_info for stream group %u is not 1\n", stg->index);
-            return AVERROR(EINVAL);
+            ret = AVERROR(EINVAL);
+            goto fail;
         }
 
         if (!param_definition) {
             param_definition = add_param_definition(iamf, param, audio_element, log_ctx);
             if (!param_definition)
-                return AVERROR(ENOMEM);
+                goto fail_enomem;
         }
     }
     if (iamf_audio_element->recon_gain_info) {
@@ -291,29 +293,36 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
 
         if (param->nb_subblocks != 1) {
             av_log(log_ctx, AV_LOG_ERROR, "nb_subblocks in recon_gain_info for stream group %u is not 1\n", stg->index);
-            return AVERROR(EINVAL);
+            ret = AVERROR(EINVAL);
+            goto fail;
         }
 
         if (!param_definition) {
             param_definition = add_param_definition(iamf, param, audio_element, log_ctx);
             if (!param_definition)
-                return AVERROR(ENOMEM);
+                goto fail_enomem;
         }
     }
 
     tmp = av_realloc_array(iamf->audio_elements, iamf->nb_audio_elements + 1, sizeof(*iamf->audio_elements));
     if (!tmp)
-        return AVERROR(ENOMEM);
+        goto fail_enomem;
 
     iamf->audio_elements = tmp;
     iamf->audio_elements[iamf->nb_audio_elements++] = audio_element;
 
     return 0;
+fail_enomem:
+    ret = AVERROR(ENOMEM);
+fail:
+    ff_iamf_free_audio_element(&audio_element);
+    return ret;
 }
 
 int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, void *log_ctx)
 {
     IAMFMixPresentation **tmp, *mix_presentation;
+    int ret;
 
     if (stg->type != AV_STREAM_GROUP_PARAMS_IAMF_MIX_PRESENTATION)
         return AVERROR(EINVAL);
@@ -340,14 +349,15 @@ int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, vo
         if (!param) {
             av_log(log_ctx, AV_LOG_ERROR, "output_mix_config is not present in submix %u from "
                                           "Mix Presentation ID %"PRId64"\n", i, stg->id);
-            return AVERROR(EINVAL);
+            ret = AVERROR(EINVAL);
+            goto fail;
         }
 
         param_definition = ff_iamf_get_param_definition(iamf, param->parameter_id);
         if (!param_definition) {
             param_definition = add_param_definition(iamf, param, NULL, log_ctx);
             if (!param_definition)
-                return AVERROR(ENOMEM);
+                goto fail_enomem;
         }
 
         for (int j = 0; j < submix->nb_elements; j++) {
@@ -357,25 +367,31 @@ int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, vo
             if (!param) {
                 av_log(log_ctx, AV_LOG_ERROR, "element_mix_config is not present for element %u in submix %u from "
                                               "Mix Presentation ID %"PRId64"\n", j, i, stg->id);
-                return AVERROR(EINVAL);
+                ret = AVERROR(EINVAL);
+                goto fail;
             }
             param_definition = ff_iamf_get_param_definition(iamf, param->parameter_id);
             if (!param_definition) {
                 param_definition = add_param_definition(iamf, param, NULL, log_ctx);
                 if (!param_definition)
-                    return AVERROR(ENOMEM);
+                    goto fail_enomem;
             }
         }
     }
 
     tmp = av_realloc_array(iamf->mix_presentations, iamf->nb_mix_presentations + 1, sizeof(*iamf->mix_presentations));
     if (!tmp)
-        return AVERROR(ENOMEM);
+        goto fail_enomem;
 
     iamf->mix_presentations = tmp;
     iamf->mix_presentations[iamf->nb_mix_presentations++] = mix_presentation;
 
     return 0;
+fail_enomem:
+    ret = AVERROR(ENOMEM);
+fail:
+    ff_iamf_free_mix_presentation(&mix_presentation);
+    return ret;
 }
 
 static int iamf_write_codec_config(const IAMFContext *iamf,
-- 
2.34.1

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

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

Re: [FFmpeg-devel] [PATCH 5/5] avformat/iamf_writer: Fix leaks on error

[James Almer]

On 2/19/2024 6:52 PM, Andreas Rheinhardt wrote:
> Fixes Coverity issues #1559544 and #1559547.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/iamf_writer.c | 42 +++++++++++++++++++++++++++------------
>   1 file changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
> index e8a88b44c3..f6ebb4aaea 100644
> --- a/libavformat/iamf_writer.c
> +++ b/libavformat/iamf_writer.c
> @@ -240,12 +240,12 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
>   
>       audio_element->substreams = av_calloc(stg->nb_streams, sizeof(*audio_element->substreams));
>       if (!audio_element->substreams)
> -        return AVERROR(ENOMEM);
> +        goto fail_enomem;
>       audio_element->nb_substreams = stg->nb_streams;
>   
>       audio_element->layers = av_calloc(iamf_audio_element->nb_layers, sizeof(*audio_element->layers));
>       if (!audio_element->layers)
> -        return AVERROR(ENOMEM);
> +        goto fail_enomem;
>   
>       for (int i = 0, j = 0; i < iamf_audio_element->nb_layers; i++) {
>           int nb_channels = iamf_audio_element->layers[i]->ch_layout.nb_channels;
> @@ -266,7 +266,8 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
>           if (nb_channels) {
>               av_log(log_ctx, AV_LOG_ERROR, "Invalid channel count across substreams in layer %u from stream group %u\n",
>                      i, stg->index);
> -            return AVERROR(EINVAL);
> +            ret = AVERROR(EINVAL);
> +            goto fail;
>           }
>       }
>   
> @@ -276,13 +277,14 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
>   
>           if (param->nb_subblocks != 1) {
>               av_log(log_ctx, AV_LOG_ERROR, "nb_subblocks in demixing_info for stream group %u is not 1\n", stg->index);
> -            return AVERROR(EINVAL);
> +            ret = AVERROR(EINVAL);
> +            goto fail;
>           }
>   
>           if (!param_definition) {
>               param_definition = add_param_definition(iamf, param, audio_element, log_ctx);
>               if (!param_definition)
> -                return AVERROR(ENOMEM);
> +                goto fail_enomem;
>           }
>       }
>       if (iamf_audio_element->recon_gain_info) {
> @@ -291,29 +293,36 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
>   
>           if (param->nb_subblocks != 1) {
>               av_log(log_ctx, AV_LOG_ERROR, "nb_subblocks in recon_gain_info for stream group %u is not 1\n", stg->index);
> -            return AVERROR(EINVAL);
> +            ret = AVERROR(EINVAL);
> +            goto fail;
>           }
>   
>           if (!param_definition) {
>               param_definition = add_param_definition(iamf, param, audio_element, log_ctx);
>               if (!param_definition)
> -                return AVERROR(ENOMEM);
> +                goto fail_enomem;
>           }
>       }
>   
>       tmp = av_realloc_array(iamf->audio_elements, iamf->nb_audio_elements + 1, sizeof(*iamf->audio_elements));
>       if (!tmp)
> -        return AVERROR(ENOMEM);
> +        goto fail_enomem;
>   
>       iamf->audio_elements = tmp;
>       iamf->audio_elements[iamf->nb_audio_elements++] = audio_element;
>   
>       return 0;
> +fail_enomem:

I prefer setting ret to ENOMEM before goto fail instead of adding a 
second label for this.

> +    ret = AVERROR(ENOMEM);
> +fail:
> +    ff_iamf_free_audio_element(&audio_element);
> +    return ret;
>   }
>   
>   int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, void *log_ctx)
>   {
>       IAMFMixPresentation **tmp, *mix_presentation;
> +    int ret;
>   
>       if (stg->type != AV_STREAM_GROUP_PARAMS_IAMF_MIX_PRESENTATION)
>           return AVERROR(EINVAL);
> @@ -340,14 +349,15 @@ int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, vo
>           if (!param) {
>               av_log(log_ctx, AV_LOG_ERROR, "output_mix_config is not present in submix %u from "
>                                             "Mix Presentation ID %"PRId64"\n", i, stg->id);
> -            return AVERROR(EINVAL);
> +            ret = AVERROR(EINVAL);
> +            goto fail;
>           }
>   
>           param_definition = ff_iamf_get_param_definition(iamf, param->parameter_id);
>           if (!param_definition) {
>               param_definition = add_param_definition(iamf, param, NULL, log_ctx);
>               if (!param_definition)
> -                return AVERROR(ENOMEM);
> +                goto fail_enomem;
>           }
>   
>           for (int j = 0; j < submix->nb_elements; j++) {
> @@ -357,25 +367,31 @@ int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, vo
>               if (!param) {
>                   av_log(log_ctx, AV_LOG_ERROR, "element_mix_config is not present for element %u in submix %u from "
>                                                 "Mix Presentation ID %"PRId64"\n", j, i, stg->id);
> -                return AVERROR(EINVAL);
> +                ret = AVERROR(EINVAL);
> +                goto fail;
>               }
>               param_definition = ff_iamf_get_param_definition(iamf, param->parameter_id);
>               if (!param_definition) {
>                   param_definition = add_param_definition(iamf, param, NULL, log_ctx);
>                   if (!param_definition)
> -                    return AVERROR(ENOMEM);
> +                    goto fail_enomem;
>               }
>           }
>       }
>   
>       tmp = av_realloc_array(iamf->mix_presentations, iamf->nb_mix_presentations + 1, sizeof(*iamf->mix_presentations));
>       if (!tmp)
> -        return AVERROR(ENOMEM);
> +        goto fail_enomem;
>   
>       iamf->mix_presentations = tmp;
>       iamf->mix_presentations[iamf->nb_mix_presentations++] = mix_presentation;
>   
>       return 0;
> +fail_enomem:
> +    ret = AVERROR(ENOMEM);
> +fail:
> +    ff_iamf_free_mix_presentation(&mix_presentation);
> +    return ret;
>   }
>   
>   static int iamf_write_codec_config(const IAMFContext *iamf,
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[FFmpeg-devel] [PATCH v2 5/6] avformat/iamf_writer: Return proper error codes

[Andreas Rheinhardt]

Surprisingly the return value of add_param_definition()
(a pointer) has only been used to check for success
and not to actually access the pointee; nonsuccess
was equated with ENOMEM, although there is a non-enomem
error path in this function.

Change this by returning an int.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
Maybe one should avoid the param_definition variables entirely
by using if (!ff_iamf_get_param_definition(...))?

 libavformat/iamf_writer.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
index e8a88b44c3..b12c7e77f9 100644
--- a/libavformat/iamf_writer.c
+++ b/libavformat/iamf_writer.c
@@ -125,8 +125,8 @@ fail:
     return ret;
 }
 
-static IAMFParamDefinition *add_param_definition(IAMFContext *iamf, AVIAMFParamDefinition *param,
-                                                 const IAMFAudioElement *audio_element, void *log_ctx)
+static int add_param_definition(IAMFContext *iamf, AVIAMFParamDefinition *param,
+                                const IAMFAudioElement *audio_element, void *log_ctx)
 {
     IAMFParamDefinition **tmp, *param_definition;
     IAMFCodecConfig *codec_config = NULL;
@@ -134,7 +134,7 @@ static IAMFParamDefinition *add_param_definition(IAMFContext *iamf, AVIAMFParamD
     tmp = av_realloc_array(iamf->param_definitions, iamf->nb_param_definitions + 1,
                            sizeof(*iamf->param_definitions));
     if (!tmp)
-        return NULL;
+        return AVERROR(ENOMEM);
 
     iamf->param_definitions = tmp;
 
@@ -145,7 +145,7 @@ static IAMFParamDefinition *add_param_definition(IAMFContext *iamf, AVIAMFParamD
         if (!codec_config) {
             av_log(log_ctx, AV_LOG_ERROR, "parameter_rate needed but not set for parameter_id %u\n",
                    param->parameter_id);
-            return NULL;
+            return AVERROR(EINVAL);
         }
         param->parameter_rate = codec_config->sample_rate;
     }
@@ -158,14 +158,14 @@ static IAMFParamDefinition *add_param_definition(IAMFContext *iamf, AVIAMFParamD
 
     param_definition = av_mallocz(sizeof(*param_definition));
     if (!param_definition)
-        return NULL;
+        return AVERROR(ENOMEM);
 
     param_definition->mode = !!param->duration;
     param_definition->param = param;
     param_definition->audio_element = audio_element;
     iamf->param_definitions[iamf->nb_param_definitions++] = param_definition;
 
-    return param_definition;
+    return 0;
 }
 
 int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void *log_ctx)
@@ -280,9 +280,9 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
         }
 
         if (!param_definition) {
-            param_definition = add_param_definition(iamf, param, audio_element, log_ctx);
-            if (!param_definition)
-                return AVERROR(ENOMEM);
+            ret = add_param_definition(iamf, param, audio_element, log_ctx);
+            if (ret < 0)
+                return ret;
         }
     }
     if (iamf_audio_element->recon_gain_info) {
@@ -295,9 +295,9 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
         }
 
         if (!param_definition) {
-            param_definition = add_param_definition(iamf, param, audio_element, log_ctx);
-            if (!param_definition)
-                return AVERROR(ENOMEM);
+            ret = add_param_definition(iamf, param, audio_element, log_ctx);
+            if (ret < 0)
+                return ret;
         }
     }
 
@@ -314,6 +314,7 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
 int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, void *log_ctx)
 {
     IAMFMixPresentation **tmp, *mix_presentation;
+    int ret;
 
     if (stg->type != AV_STREAM_GROUP_PARAMS_IAMF_MIX_PRESENTATION)
         return AVERROR(EINVAL);
@@ -345,9 +346,9 @@ int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, vo
 
         param_definition = ff_iamf_get_param_definition(iamf, param->parameter_id);
         if (!param_definition) {
-            param_definition = add_param_definition(iamf, param, NULL, log_ctx);
-            if (!param_definition)
-                return AVERROR(ENOMEM);
+            ret = add_param_definition(iamf, param, NULL, log_ctx);
+            if (ret < 0)
+                return ret;
         }
 
         for (int j = 0; j < submix->nb_elements; j++) {
@@ -361,9 +362,9 @@ int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, vo
             }
             param_definition = ff_iamf_get_param_definition(iamf, param->parameter_id);
             if (!param_definition) {
-                param_definition = add_param_definition(iamf, param, NULL, log_ctx);
-                if (!param_definition)
-                    return AVERROR(ENOMEM);
+                ret = add_param_definition(iamf, param, NULL, log_ctx);
+                if (ret < 0)
+                    return ret;
             }
         }
     }
-- 
2.34.1

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

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

Re: [FFmpeg-devel] [PATCH v2 5/6] avformat/iamf_writer: Return proper error codes

[James Almer]

On 2/19/2024 7:17 PM, Andreas Rheinhardt wrote:
> Surprisingly the return value of add_param_definition()
> (a pointer) has only been used to check for success
> and not to actually access the pointee; nonsuccess
> was equated with ENOMEM, although there is a non-enomem
> error path in this function.
> 
> Change this by returning an int.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> Maybe one should avoid the param_definition variables entirely
> by using if (!ff_iamf_get_param_definition(...))?

Sure, either way (this patch or this suggestion) is fine.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

[FFmpeg-devel] [PATCH v2 6/6] avformat/iamf_writer: Fix leaks on error

[Andreas Rheinhardt]

Fixes Coverity issues #1559544 and #1559547.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavformat/iamf_writer.c | 53 ++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 17 deletions(-)

diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
index b12c7e77f9..a807fed786 100644
--- a/libavformat/iamf_writer.c
+++ b/libavformat/iamf_writer.c
@@ -239,13 +239,17 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
     audio_element->codec_config_id = ret;
 
     audio_element->substreams = av_calloc(stg->nb_streams, sizeof(*audio_element->substreams));
-    if (!audio_element->substreams)
-        return AVERROR(ENOMEM);
+    if (!audio_element->substreams) {
+        ret = AVERROR(ENOMEM);
+        goto fail;
+    }
     audio_element->nb_substreams = stg->nb_streams;
 
     audio_element->layers = av_calloc(iamf_audio_element->nb_layers, sizeof(*audio_element->layers));
-    if (!audio_element->layers)
-        return AVERROR(ENOMEM);
+    if (!audio_element->layers) {
+        ret = AVERROR(ENOMEM);
+        goto fail;
+    }
 
     for (int i = 0, j = 0; i < iamf_audio_element->nb_layers; i++) {
         int nb_channels = iamf_audio_element->layers[i]->ch_layout.nb_channels;
@@ -266,7 +270,8 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
         if (nb_channels) {
             av_log(log_ctx, AV_LOG_ERROR, "Invalid channel count across substreams in layer %u from stream group %u\n",
                    i, stg->index);
-            return AVERROR(EINVAL);
+            ret = AVERROR(EINVAL);
+            goto fail;
         }
     }
 
@@ -276,13 +281,14 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
 
         if (param->nb_subblocks != 1) {
             av_log(log_ctx, AV_LOG_ERROR, "nb_subblocks in demixing_info for stream group %u is not 1\n", stg->index);
-            return AVERROR(EINVAL);
+            ret = AVERROR(EINVAL);
+            goto fail;
         }
 
         if (!param_definition) {
             ret = add_param_definition(iamf, param, audio_element, log_ctx);
             if (ret < 0)
-                return ret;
+                goto fail;
         }
     }
     if (iamf_audio_element->recon_gain_info) {
@@ -291,24 +297,30 @@ int ff_iamf_add_audio_element(IAMFContext *iamf, const AVStreamGroup *stg, void
 
         if (param->nb_subblocks != 1) {
             av_log(log_ctx, AV_LOG_ERROR, "nb_subblocks in recon_gain_info for stream group %u is not 1\n", stg->index);
-            return AVERROR(EINVAL);
+            ret = AVERROR(EINVAL);
+            goto fail;
         }
 
         if (!param_definition) {
             ret = add_param_definition(iamf, param, audio_element, log_ctx);
             if (ret < 0)
-                return ret;
+                goto fail;
         }
     }
 
     tmp = av_realloc_array(iamf->audio_elements, iamf->nb_audio_elements + 1, sizeof(*iamf->audio_elements));
-    if (!tmp)
-        return AVERROR(ENOMEM);
+    if (!tmp) {
+        ret = AVERROR(ENOMEM);
+        goto fail;
+    }
 
     iamf->audio_elements = tmp;
     iamf->audio_elements[iamf->nb_audio_elements++] = audio_element;
 
     return 0;
+fail:
+    ff_iamf_free_audio_element(&audio_element);
+    return ret;
 }
 
 int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, void *log_ctx)
@@ -341,14 +353,15 @@ int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, vo
         if (!param) {
             av_log(log_ctx, AV_LOG_ERROR, "output_mix_config is not present in submix %u from "
                                           "Mix Presentation ID %"PRId64"\n", i, stg->id);
-            return AVERROR(EINVAL);
+            ret = AVERROR(EINVAL);
+            goto fail;
         }
 
         param_definition = ff_iamf_get_param_definition(iamf, param->parameter_id);
         if (!param_definition) {
             ret = add_param_definition(iamf, param, NULL, log_ctx);
             if (ret < 0)
-                return ret;
+                goto fail;
         }
 
         for (int j = 0; j < submix->nb_elements; j++) {
@@ -358,25 +371,31 @@ int ff_iamf_add_mix_presentation(IAMFContext *iamf, const AVStreamGroup *stg, vo
             if (!param) {
                 av_log(log_ctx, AV_LOG_ERROR, "element_mix_config is not present for element %u in submix %u from "
                                               "Mix Presentation ID %"PRId64"\n", j, i, stg->id);
-                return AVERROR(EINVAL);
+                ret = AVERROR(EINVAL);
+                goto fail;
             }
             param_definition = ff_iamf_get_param_definition(iamf, param->parameter_id);
             if (!param_definition) {
                 ret = add_param_definition(iamf, param, NULL, log_ctx);
                 if (ret < 0)
-                    return ret;
+                    goto fail;
             }
         }
     }
 
     tmp = av_realloc_array(iamf->mix_presentations, iamf->nb_mix_presentations + 1, sizeof(*iamf->mix_presentations));
-    if (!tmp)
-        return AVERROR(ENOMEM);
+    if (!tmp) {
+        ret = AVERROR(ENOMEM);
+        goto fail;
+    }
 
     iamf->mix_presentations = tmp;
     iamf->mix_presentations[iamf->nb_mix_presentations++] = mix_presentation;
 
     return 0;
+fail:
+    ff_iamf_free_mix_presentation(&mix_presentation);
+    return ret;
 }
 
 static int iamf_write_codec_config(const IAMFContext *iamf,
-- 
2.34.1

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

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

Re: [FFmpeg-devel] [PATCH v2 6/6] avformat/iamf_writer: Fix leaks on error

[James Almer]

On 2/19/2024 7:17 PM, Andreas Rheinhardt wrote:
> Fixes Coverity issues #1559544 and #1559547.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/iamf_writer.c | 53 ++++++++++++++++++++++++++-------------
>   1 file changed, 36 insertions(+), 17 deletions(-)

LGTM, thanks.
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

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

Re: [FFmpeg-devel] [PATCH 1/5] avformat/iamf_writer: Don't leak on error when adding ParamDefinition

[James Almer]

On 2/19/2024 6:51 PM, Andreas Rheinhardt wrote:
> Fix this by postponing the allocation.
> Fixes Coverity issue #1559545.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavformat/iamf_writer.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/libavformat/iamf_writer.c b/libavformat/iamf_writer.c
> index 1a360dee2f..897b06310f 100644
> --- a/libavformat/iamf_writer.c
> +++ b/libavformat/iamf_writer.c
> @@ -138,10 +138,6 @@ static IAMFParamDefinition *add_param_definition(IAMFContext *iamf, AVIAMFParamD
>   
>       iamf->param_definitions = tmp;
>   
> -    param_definition = av_mallocz(sizeof(*param_definition));
> -    if (!param_definition)
> -        return NULL;
> -
>       if (audio_element)
>           codec_config = iamf->codec_configs[audio_element->codec_config_id];
>   
> @@ -160,6 +156,10 @@ static IAMFParamDefinition *add_param_definition(IAMFContext *iamf, AVIAMFParamD
>               param->constant_subblock_duration = codec_config->nb_samples;
>       }
>   
> +    param_definition = av_mallocz(sizeof(*param_definition));
> +    if (!param_definition)
> +        return NULL;
> +
>       param_definition->mode = !!param->duration;
>       param_definition->param = param;
>       param_definition->audio_element = audio_element;

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

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