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 at tsinghua.edu.cn>
> Signed-off-by: Yu Yang <young_chelsea at 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.
- Andreas