| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
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: > 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 --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,
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavformat/vividas.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)