diff mbox series

[FFmpeg-devel] libavcodec/avpacketc packet release exception

Message ID 20211203093911.65914-1-young_chelsea@163.com
State New
Headers show
Series [FFmpeg-devel] libavcodec/avpacketc packet release exception
Related show

Checks

Context Check Description
andriy/commit_msg_x86 warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/commit_msg_ppc warning The first line of the commit message must start with a context terminated by a colon and a space, for example "lavu/opt: " or "doc: ".
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Yy Dec. 3, 2021, 9:39 a.m. UTC
'pkt' and '*pkt' should be judged separately for release. 
SEGV by a READ memory access (address points to the zero page) 

```c
// in fftools/ffmpeg.c:515
 515 static void ffmpeg_cleanup(int ret)
 516 {
       ...

 626     for (i = 0; i < nb_input_files; i++) {
 627         avformat_close_input(&input_files[i]->ctx);
             // `input_files[0] == NULL` but `&input_files[0]->pkt == 0x68`, see below;
 628         av_packet_free(&input_files[i]->pkt);
 629         av_freep(&input_files[i]);
 630     }

       ...
 674 }
```
```c
// in libavcodec/avpacket.c:75
 75 void av_packet_free(AVPacket **pkt)
 76 {
       // pkt == 0x68, `*pkt` cause `SEGV`.
 77     if (!pkt || !*pkt)
 78         return;
 79 
 80     av_packet_unref(*pkt);
 81     av_freep(pkt);
 82 }
```

coredump backtrace info:
==4536==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000068 (pc 0x000002a794d0 bp 0x7ffdf587a910 sp 0x7ffdf587a8e0 T0)
==4536==The signal is caused by a READ memory access.
==4536==Hint: address points to the zero page.
    #0 0x2a794d0 in av_packet_free /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavcodec/avpacket.c:77:18
    #1 0x592107 in ffmpeg_cleanup /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:628:9
    #2 0x55fe0e in exit_program /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/cmdutils.c:136:9
    #3 0x4cfcd4 in open_input_file /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:1268:9  exit_program
    #4 0x4c9dc0 in open_files /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3338:15     
    #5 0x4c9295 in ffmpeg_parse_options /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3378:11 open_file
    #6 0x58f241 in main /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4988:11
    #7 0x7f122a83d0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #8 0x42033d in _start (/home/r1/ffmpeg/ffmpeg_4.4.1+0x42033d)


Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
Signed-off-by: Yu Yang <young_chelsea@163.com>
---
 libavcodec/avpacket.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Yy Dec. 3, 2021, 12:56 p.m. UTC | #1
> 2021年12月3日 下午5:45,Andreas Rheinhardt <andreas.rheinhardt@outlook.com> 写道:
> 
> Yu Yang:
>> 'pkt' and '*pkt' should be judged separately for release. 
>> SEGV by a READ memory access (address points to the zero page) 
>> 
>> ```c
>> // in fftools/ffmpeg.c:515
>> 515 static void ffmpeg_cleanup(int ret)
>> 516 {
>>       ...
>> 
>> 626     for (i = 0; i < nb_input_files; i++) {
>> 627         avformat_close_input(&input_files[i]->ctx);
>>             // `input_files[0] == NULL` but `&input_files[0]->pkt == 0x68`, see below;
>> 628         av_packet_free(&input_files[i]->pkt);
>> 629         av_freep(&input_files[i]);
>> 630     }
>> 
>>       ...
>> 674 }
>> ```
>> ```c
>> // in libavcodec/avpacket.c:75
>> 75 void av_packet_free(AVPacket **pkt)
>> 76 {
>>       // pkt == 0x68, `*pkt` cause `SEGV`.
>> 77     if (!pkt || !*pkt)
>> 78         return;
>> 79 
>> 80     av_packet_unref(*pkt);
>> 81     av_freep(pkt);
>> 82 }
>> ```
>> 
>> coredump backtrace info:
>> ==4536==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000068 (pc 0x000002a794d0 bp 0x7ffdf587a910 sp 0x7ffdf587a8e0 T0)
>> ==4536==The signal is caused by a READ memory access.
>> ==4536==Hint: address points to the zero page.
>>    #0 0x2a794d0 in av_packet_free /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavcodec/avpacket.c:77:18
>>    #1 0x592107 in ffmpeg_cleanup /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:628:9
>>    #2 0x55fe0e in exit_program /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/cmdutils.c:136:9
>>    #3 0x4cfcd4 in open_input_file /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:1268:9  exit_program
>>    #4 0x4c9dc0 in open_files /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3338:15     
>>    #5 0x4c9295 in ffmpeg_parse_options /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_opt.c:3378:11 open_file
>>    #6 0x58f241 in main /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4988:11
>>    #7 0x7f122a83d0b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
>>    #8 0x42033d in _start (/home/r1/ffmpeg/ffmpeg_4.4.1+0x42033d)
>> 
>> 
>> Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
>> Signed-off-by: Yu Yang <young_chelsea@163.com>
>> ---
>> libavcodec/avpacket.c | 9 ++++-----
>> 1 file changed, 4 insertions(+), 5 deletions(-)
>> 
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index d8d8fef3b9..8348bec581 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -74,11 +74,10 @@ AVPacket *av_packet_alloc(void)
>> 
>> void av_packet_free(AVPacket **pkt)
>> {
>> -    if (!pkt || !*pkt)
>> -        return;
>> -
>> -    av_packet_unref(*pkt);
>> -    av_freep(pkt);
>> +    if (*pkt)
>> +        av_packet_unref(*pkt);
>> +    if (pkt)
>> +        av_freep(pkt);
>> }
>> 
>> static int packet_alloc(AVBufferRef **buf, int size)
>> 
> 
> This change makes no sense at all: If pkt == NULL (this should actually
> never happen and indicates a bug somewhere else; we should actually not
> check for this, but legacy), then the first check dereferences a NULL
> pointer and crashes.
Thx, the code I wrote is indeed too stupid.
> 
> - Andreas
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index d8d8fef3b9..8348bec581 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -74,11 +74,10 @@  AVPacket *av_packet_alloc(void)
 
 void av_packet_free(AVPacket **pkt)
 {
-    if (!pkt || !*pkt)
-        return;
-
-    av_packet_unref(*pkt);
-    av_freep(pkt);
+    if (*pkt)
+        av_packet_unref(*pkt);
+    if (pkt)
+        av_freep(pkt);
 }
 
 static int packet_alloc(AVBufferRef **buf, int size)