Message ID | 20200104190005.29682-1-george@nsup.org |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] lavc/avpacket: forward av_buffer_realloc() error code. | expand |
Context | Check | Description |
---|---|---|
andriy/ffmpeg-patchwork | success | Make fate finished |
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) >
Nicolas George (12020-01-04): > Fix CID 1457229. > > Signed-off-by: Nicolas George <george@nsup.org> > --- > libavcodec/avpacket.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) Ping? Will apply next time I think about it if nobody objects and it still applies. Regards,
Nicolas George (12020-08-09): > Ping? Will apply next time I think about it if nobody objects and it > still applies. Sorry, I missed Andreas comment in another mailbox. Regards,
On 1/4/2020 4:00 PM, Nicolas George wrote: > 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); \ > 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) This code is deprecated and will be removed in the next major bump, so IMO it's worth trying to fix it, and especially if it's just to silence a static analyzer.
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)
Fix CID 1457229. Signed-off-by: Nicolas George <george@nsup.org> --- libavcodec/avpacket.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)