diff mbox series

[FFmpeg-devel,2/3] avformat/vividas: Check allocation for success

Message ID 20200805233358.31711-2-andreas.rheinhardt@gmail.com
State Accepted
Commit c4a4fe938d435de9e9126d7e151fc370a6f5ee72
Headers show
Series [FFmpeg-devel,1/3] avformat/vividas: Check return value before storing it in smaller type
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 Aug. 5, 2020, 11:33 p.m. UTC
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavformat/vividas.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

Comments

Zane van Iperen Aug. 6, 2020, 1:09 a.m. UTC | #1
On Thu, 6 Aug 2020 01:33:57 +0200
"Andreas Rheinhardt" <andreas.rheinhardt@gmail.com> wrote:

> 
>  static void load_sb_block(AVFormatContext *s, VividasDemuxContext *viv, unsigned expected_size)
> @@ -608,7 +606,7 @@ static int viv_read_header(AVFormatContext *s)
>      ret = track_index(viv, s, buf, v);
>      av_free(buf);
>      if (ret < 0)
> -        return ret;
> +        goto fail;
> 
>      viv->sb_offset = avio_tell(pb);
>      if (viv->n_sb_blocks > 0) {
> @@ -619,6 +617,9 @@ static int viv_read_header(AVFormatContext *s)
>      }
> 
>      return 0;
> +fail:
> +    av_freep(&viv->sb_blocks);
> +    return ret;
>  }

Nit: Considering there's only one `goto fail`, wouldn't it be better to
just av_freep and return immediately instead?

Otherwise, all lgtm.

Zane
Andreas Rheinhardt Aug. 6, 2020, 9:29 a.m. UTC | #2
Zane van Iperen:
> On Thu, 6 Aug 2020 01:33:57 +0200
> "Andreas Rheinhardt" <andreas.rheinhardt@gmail.com> wrote:
> 
>>
>>  static void load_sb_block(AVFormatContext *s, VividasDemuxContext *viv, unsigned expected_size)
>> @@ -608,7 +606,7 @@ static int viv_read_header(AVFormatContext *s)
>>      ret = track_index(viv, s, buf, v);
>>      av_free(buf);
>>      if (ret < 0)
>> -        return ret;
>> +        goto fail;
>>
>>      viv->sb_offset = avio_tell(pb);
>>      if (viv->n_sb_blocks > 0) {
>> @@ -619,6 +617,9 @@ static int viv_read_header(AVFormatContext *s)
>>      }
>>
>>      return 0;
>> +fail:
>> +    av_freep(&viv->sb_blocks);
>> +    return ret;
>>  }
> 
> Nit: Considering there's only one `goto fail`, wouldn't it be better to
> just av_freep and return immediately instead?
> 
This patch is designed so that this demuxer can easily be converted to
automatically call read_close on read_header failure (see [1] for
details, in fact all bugs in this patchset have been found when working
on this goal (I already have 61 demuxers now (17 more to go in
libavformat plus all the demuxers in libavdevice)). Doing cleanup the
way I do it here implies that the patch that marks this demuxer as
compatible with automatic freeing can simply remove the whole fail part
above and replace "goto fail" with return ret again. But if I cleaned up
in place, I would have to touch the "if (ret < 0)" line now and either
unnecessarily touch it again when the cleanup will be made automatic or
keep the unnecessary {} in "if (ret < 0) {\n return ret;\n}".

- Andreas
Andreas Rheinhardt Aug. 6, 2020, 9:30 a.m. UTC | #3
Andreas Rheinhardt:
> Zane van Iperen:
>> On Thu, 6 Aug 2020 01:33:57 +0200
>> "Andreas Rheinhardt" <andreas.rheinhardt@gmail.com> wrote:
>>
>>>
>>>  static void load_sb_block(AVFormatContext *s, VividasDemuxContext *viv, unsigned expected_size)
>>> @@ -608,7 +606,7 @@ static int viv_read_header(AVFormatContext *s)
>>>      ret = track_index(viv, s, buf, v);
>>>      av_free(buf);
>>>      if (ret < 0)
>>> -        return ret;
>>> +        goto fail;
>>>
>>>      viv->sb_offset = avio_tell(pb);
>>>      if (viv->n_sb_blocks > 0) {
>>> @@ -619,6 +617,9 @@ static int viv_read_header(AVFormatContext *s)
>>>      }
>>>
>>>      return 0;
>>> +fail:
>>> +    av_freep(&viv->sb_blocks);
>>> +    return ret;
>>>  }
>>
>> Nit: Considering there's only one `goto fail`, wouldn't it be better to
>> just av_freep and return immediately instead?
>>
> This patch is designed so that this demuxer can easily be converted to
> automatically call read_close on read_header failure (see [1] for
> details, in fact all bugs in this patchset have been found when working
> on this goal (I already have 61 demuxers now (17 more to go in
> libavformat plus all the demuxers in libavdevice)). Doing cleanup the
> way I do it here implies that the patch that marks this demuxer as
> compatible with automatic freeing can simply remove the whole fail part
> above and replace "goto fail" with return ret again. But if I cleaned up
> in place, I would have to touch the "if (ret < 0)" line now and either
> unnecessarily touch it again when the cleanup will be made automatic or
> keep the unnecessary {} in "if (ret < 0) {\n return ret;\n}".
> 
> - Andreas
> 
[1] of course refers to
https://ffmpeg.org/pipermail/ffmpeg-devel/2020-July/266575.html. Forgot
to add it.

- Andreas
diff mbox series

Patch

diff --git a/libavformat/vividas.c b/libavformat/vividas.c
index 708adc8801..36c007b0d2 100644
--- a/libavformat/vividas.c
+++ b/libavformat/vividas.c
@@ -440,7 +440,7 @@  static int track_index(VividasDemuxContext *viv, AVFormatContext *s, uint8_t *bu
     avio_r8(pb); // 'c'
     n_sb_blocks_tmp = ffio_read_varlen(pb);
     if (n_sb_blocks_tmp > size / 2)
-        goto error;
+        return AVERROR_INVALIDDATA;
     viv->sb_blocks = av_calloc(n_sb_blocks_tmp, sizeof(*viv->sb_blocks));
     if (!viv->sb_blocks) {
         return AVERROR(ENOMEM);
@@ -455,7 +455,7 @@  static int track_index(VividasDemuxContext *viv, AVFormatContext *s, uint8_t *bu
         uint64_t n_packets_tmp = ffio_read_varlen(pb);
 
         if (size_tmp > INT_MAX || n_packets_tmp > INT_MAX)
-            goto error;
+            return AVERROR_INVALIDDATA;
 
         viv->sb_blocks[i].byte_offset = off;
         viv->sb_blocks[i].packet_offset = poff;
@@ -471,15 +471,13 @@  static int track_index(VividasDemuxContext *viv, AVFormatContext *s, uint8_t *bu
     }
 
     if (filesize > 0 && poff > filesize)
-        goto error;
+        return AVERROR_INVALIDDATA;
 
     viv->sb_entries = av_calloc(maxnp, sizeof(VIV_SB_entry));
+    if (!viv->sb_entries)
+        return AVERROR(ENOMEM);
 
     return 0;
-error:
-    viv->n_sb_blocks = 0;
-    av_freep(&viv->sb_blocks);
-    return AVERROR_INVALIDDATA;
 }
 
 static void load_sb_block(AVFormatContext *s, VividasDemuxContext *viv, unsigned expected_size)
@@ -608,7 +606,7 @@  static int viv_read_header(AVFormatContext *s)
     ret = track_index(viv, s, buf, v);
     av_free(buf);
     if (ret < 0)
-        return ret;
+        goto fail;
 
     viv->sb_offset = avio_tell(pb);
     if (viv->n_sb_blocks > 0) {
@@ -619,6 +617,9 @@  static int viv_read_header(AVFormatContext *s)
     }
 
     return 0;
+fail:
+    av_freep(&viv->sb_blocks);
+    return ret;
 }
 
 static int viv_read_packet(AVFormatContext *s,