Opened 11 months ago

Last modified 8 months ago

#7484 new defect

av_packet_ref(): Allocates array on zero src size

Reported by: zerodefect Owned by:
Priority: normal Component: avcodec
Version: git-master Keywords:
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description

Discovered a bug on a corner-case when exercising some unit tests I had written. The code involved pertains to function:

int av_packet_ref(AVPacket *dst, const AVPacket *src).

I'll jump right into the code example:

Code highlighting:

#include <cassert>

extern "C"
{
#include <libavcodec/avcodec.h>
}

int main(int argc, char *argv[])
{
        AVPacket pkt{};
        av_init_packet(&pkt);
        assert(pkt.size == 0); // OK;
        assert(pkt.data == nullptr); // OK
        
        AVPacket pkt2{};
        av_init_packet(&pkt2);
        assert(pkt2.size == 0); // OK;
        assert(pkt2.data == nullptr); // OK
        
        assert(0 == av_packet_ref(&pkt2, &pkt)); // OK? Discuss.
        
        assert(pkt.size == 0); // OK;
        assert(pkt.data == nullptr); // OK
        
        assert(pkt2.size == 0); // OK;
        assert(pkt2.data == nullptr); // ASSERTS: Woah! Not good! 

        return 0;
}

So av_packet_ref(...) func is called with a Src and Dst pkt whose size is both 0 and data is both NULL, yet after the function is called the data on the dst is non-NULL.

Code sample built using the following command:
g++ main.cpp -lavcodec -o test

Ubuntu 18.04
GCC 8.2
FFmpeg v4.0.2

Change History (2)

comment:1 Changed 9 months ago by zerodefect

I took a stab at this over the end-of-year break. I think the best way to solve this is to disable allocation of an AVPacket with size of 0:

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index e160ad3033..df4cafc6da 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -71,7 +71,7 @@ void av_packet_free(AVPacket **pkt)
 static int packet_alloc(AVBufferRef **buf, int size)
 {
     int ret;
-    if (size < 0 || size >= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
+    if (size <= 0 || size >= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
         return AVERROR(EINVAL);
 
     ret = av_buffer_realloc(buf, size + AV_INPUT_BUFFER_PADDING_SIZE);

I applied the above patch to latest.

As per the development guidelines, I tried out the fate suite of tests, but I notice that the acodec-flac test fails which could be bad. I tried to follow the make file, but I struggled to quite understand how fate works in order to debug.

comment:2 Changed 8 months ago by cehoyos

  • Version set to git-master

Why do you believe the current behaviour is wrong? Is it documented differently?
It seems that at least ff_read_packet() expects the current behaviour when doing err = av_packet_make_refcounted(pkt);

If you want fix the failing fate tests, you can replace return AVERROR(EINVAL) with abort() to find the offending calls.

Note: See TracTickets for help on using tickets.