diff mbox series

[FFmpeg-devel] lavc/avpacket: forward av_buffer_realloc() error code.

Message ID 20200104190005.29682-1-george@nsup.org
State New
Headers show
Series [FFmpeg-devel] lavc/avpacket: forward av_buffer_realloc() error code.
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

Nicolas George Jan. 4, 2020, 7 p.m. UTC
Fix CID 1457229.

Signed-off-by: Nicolas George <george@nsup.org>
---
 libavcodec/avpacket.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Andreas Rheinhardt Jan. 4, 2020, 7:25 p.m. UTC | #1
Nicolas George:
> Fix CID 1457229.
> 
> Signed-off-by: Nicolas George <george@nsup.org>
> ---
>  libavcodec/avpacket.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 858f827a0a..4da832c53c 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -170,7 +170,7 @@ FF_DISABLE_DEPRECATION_WARNINGS
>  #define ALLOC_MALLOC(data, size) data = av_malloc(size)
>  #define ALLOC_BUF(data, size)                \
>  do {                                         \
> -    av_buffer_realloc(&pkt->buf, size);      \
> +    ret = av_buffer_realloc(&pkt->buf, size);      \

You do not change the check, so the same comment as to your other
patch applies: Do you think that Coverity will think that this is a
real check? (I don't think so, otherwise it wouldn't have raised the
issue in the first place.)

>      data = pkt->buf ? pkt->buf->data : NULL; \
>  } while (0)
>  
> @@ -197,6 +197,8 @@ do {                                         \
>  /* Makes duplicates of data, side_data, but does not copy any other fields */
>  static int copy_packet_data(AVPacket *pkt, const AVPacket *src, int dup)
>  {
> +    int ret = AVERROR_BUG;
> +
>      pkt->data      = NULL;
>      pkt->side_data = NULL;
>      pkt->side_data_elems = 0;
> @@ -220,7 +222,8 @@ static int copy_packet_data(AVPacket *pkt, const AVPacket *src, int dup)
>  
>  failed_alloc:
>      av_packet_unref(pkt);
> -    return AVERROR(ENOMEM);
> +    av_assert1(ret != AVERROR_BUG);

There is an overflow check that could also trigger going to
failed_alloc. The overflow check is very weird: It is only triggered
if pkt->size is in the range -AV_INPUT_BUFFER_PADDING_SIZE..-1 and so
doesn't do its job (should be replaced by (unsigned)(size) > INT_MAX -
AV_INPUT_BUFFER_PADDING_SIZE). But if it were fixed, one could run
into this assert. How about initializing ret to 0 and setting ret to
AVERROR(ERANGE) if it is still zero in failed_alloc?

- Andreas

> +    return ret;
>  }
>  
>  int av_copy_packet_side_data(AVPacket *pkt, const AVPacket *src)
>
diff mbox series

Patch

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index 858f827a0a..4da832c53c 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -170,7 +170,7 @@  FF_DISABLE_DEPRECATION_WARNINGS
 #define ALLOC_MALLOC(data, size) data = av_malloc(size)
 #define ALLOC_BUF(data, size)                \
 do {                                         \
-    av_buffer_realloc(&pkt->buf, size);      \
+    ret = av_buffer_realloc(&pkt->buf, size);      \
     data = pkt->buf ? pkt->buf->data : NULL; \
 } while (0)
 
@@ -197,6 +197,8 @@  do {                                         \
 /* Makes duplicates of data, side_data, but does not copy any other fields */
 static int copy_packet_data(AVPacket *pkt, const AVPacket *src, int dup)
 {
+    int ret = AVERROR_BUG;
+
     pkt->data      = NULL;
     pkt->side_data = NULL;
     pkt->side_data_elems = 0;
@@ -220,7 +222,8 @@  static int copy_packet_data(AVPacket *pkt, const AVPacket *src, int dup)
 
 failed_alloc:
     av_packet_unref(pkt);
-    return AVERROR(ENOMEM);
+    av_assert1(ret != AVERROR_BUG);
+    return ret;
 }
 
 int av_copy_packet_side_data(AVPacket *pkt, const AVPacket *src)