diff mbox series

[FFmpeg-devel] Exception when frame is set NULL

Message ID 20211203094343.66139-1-young_chelsea@163.com
State New
Headers show
Series [FFmpeg-devel] Exception when frame is set NULL
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:43 a.m. UTC
fftools/ffmpegc  When `ost->last_frame` is NULL, 'SEGV' occurs when accessing its pts.
                 
libavutil/framec `ost->last_frame` may be set NULL by av_frame_alloc(). In this situation,
                 av_frame_unref() and av_frame_free() do nothing. Frame is not released.

```c
// in fftools/ffmpeg.c:1145
1145 static void do_video_out(OutputFile *of, ...)

1148 {
      ...
             // `ost->last_frame` is NULL.
1272         av_log(NULL, AV_LOG_VERBOSE,
1273                "*** dropping frame %d from stream %d at ts %"PRId64"\n",
1274                ost->frame_number, ost->st->index, ost->last_frame->pts);
      ...
1421     if (!ost->last_frame)
             // `ost->last_frame` may be set NULL here.
1422         ost->last_frame = av_frame_alloc();
      ...

1433 }
```

coredump backtrace info:
==7192==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000088 (pc 0x0000005e87e2 bp 0x7fff84f0ffb0 sp 0x7fff84f0f020 T0)
==7192==The signal is caused by a READ memory access.
==7192==Hint: address points to the zero page.
    #0 0x5e87e2 in do_video_out /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:1274:68
    #1 0x5df341 in reap_filters /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:1548:25
    #2 0x5d08b7 in transcode_from_filter /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4644:15
    #3 0x59e557 in transcode_step /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4729:20
    #4 0x593970 in transcode /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4805:15
    #5 0x58f7a4 in main /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:5010:9
    #6 0x7f0fa9d900b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #7 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>
---
 fftools/ffmpeg.c  | 7 ++++---
 libavutil/frame.c | 9 ++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

Comments

Andreas Rheinhardt Dec. 3, 2021, 10:04 a.m. UTC | #1
Yu Yang:
> fftools/ffmpegc  When `ost->last_frame` is NULL, 'SEGV' occurs when accessing its pts.
>                  
> libavutil/framec `ost->last_frame` may be set NULL by av_frame_alloc(). In this situation,
>                  av_frame_unref() and av_frame_free() do nothing. Frame is not released.
> 
> ```c
> // in fftools/ffmpeg.c:1145
> 1145 static void do_video_out(OutputFile *of, ...)
> 
> 1148 {
>       ...
>              // `ost->last_frame` is NULL.
> 1272         av_log(NULL, AV_LOG_VERBOSE,
> 1273                "*** dropping frame %d from stream %d at ts %"PRId64"\n",
> 1274                ost->frame_number, ost->st->index, ost->last_frame->pts);
>       ...
> 1421     if (!ost->last_frame)
>              // `ost->last_frame` may be set NULL here.
> 1422         ost->last_frame = av_frame_alloc();
>       ...
> 
> 1433 }
> ```
> 
> coredump backtrace info:
> ==7192==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000088 (pc 0x0000005e87e2 bp 0x7fff84f0ffb0 sp 0x7fff84f0f020 T0)
> ==7192==The signal is caused by a READ memory access.
> ==7192==Hint: address points to the zero page.
>     #0 0x5e87e2 in do_video_out /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:1274:68
>     #1 0x5df341 in reap_filters /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:1548:25
>     #2 0x5d08b7 in transcode_from_filter /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4644:15
>     #3 0x59e557 in transcode_step /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4729:20
>     #4 0x593970 in transcode /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4805:15
>     #5 0x58f7a4 in main /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:5010:9
>     #6 0x7f0fa9d900b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
>     #7 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>
> ---
>  fftools/ffmpeg.c  | 7 ++++---
>  libavutil/frame.c | 9 ++++-----
>  2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
> index cfb04d5eff..cade05f762 100644
> --- a/fftools/ffmpeg.c
> +++ b/fftools/ffmpeg.c
> @@ -1265,9 +1265,10 @@ static void do_video_out(OutputFile *of,
>  
>      if (nb0_frames == 0 && ost->last_dropped) {
>          nb_frames_drop++;
> -        av_log(NULL, AV_LOG_VERBOSE,
> -               "*** dropping frame %d from stream %d at ts %"PRId64"\n",
> -               ost->frame_number, ost->st->index, ost->last_frame->pts);
> +        if (ost->last_frame)
> +            av_log(NULL, AV_LOG_VERBOSE,
> +                   "*** dropping frame %d from stream %d at ts %"PRId64"\n",
> +                   ost->frame_number, ost->st->index, ost->last_frame->pts);
>      }
>      if (nb_frames > (nb0_frames && ost->last_dropped) + (nb_frames > nb0_frames)) {
>          if (nb_frames > dts_error_threshold * 30) {
> diff --git a/libavutil/frame.c b/libavutil/frame.c
> index d4d3ad6988..9c866320a7 100644
> --- a/libavutil/frame.c
> +++ b/libavutil/frame.c
> @@ -111,11 +111,10 @@ AVFrame *av_frame_alloc(void)
>  
>  void av_frame_free(AVFrame **frame)
>  {
> -    if (!frame || !*frame)
> -        return;
> -
> -    av_frame_unref(*frame);
> -    av_freep(frame);
> +    if (*frame)
> +        av_frame_unref(*frame);
> +    if (frame)
> +        av_freep(frame);
>  }
>  
>  static int get_video_buffer(AVFrame *frame, int align)
> 

This change to frame.c is also completely wrong; this frame should
probably not be constantly allocated and freed and the code at lines
1422-1428 should actually error out in case of allocation error.

- Andreas
Yy Dec. 3, 2021, 1:12 p.m. UTC | #2
> 2021年12月3日 下午6:04,Andreas Rheinhardt <andreas.rheinhardt@outlook.com> 写道:
> 
> Yu Yang:
>> fftools/ffmpegc  When `ost->last_frame` is NULL, 'SEGV' occurs when accessing its pts.
>> 
>> libavutil/framec `ost->last_frame` may be set NULL by av_frame_alloc(). In this situation,
>>                 av_frame_unref() and av_frame_free() do nothing. Frame is not released.
>> 
>> ```c
>> // in fftools/ffmpeg.c:1145
>> 1145 static void do_video_out(OutputFile *of, ...)
>> 
>> 1148 {
>>      ...
>>             // `ost->last_frame` is NULL.
>> 1272         av_log(NULL, AV_LOG_VERBOSE,
>> 1273                "*** dropping frame %d from stream %d at ts %"PRId64"\n",
>> 1274                ost->frame_number, ost->st->index, ost->last_frame->pts);
>>      ...
>> 1421     if (!ost->last_frame)
>>             // `ost->last_frame` may be set NULL here.
>> 1422         ost->last_frame = av_frame_alloc();
>>      ...
>> 
>> 1433 }
>> ```
>> 
>> coredump backtrace info:
>> ==7192==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000088 (pc 0x0000005e87e2 bp 0x7fff84f0ffb0 sp 0x7fff84f0f020 T0)
>> ==7192==The signal is caused by a READ memory access.
>> ==7192==Hint: address points to the zero page.
>>    #0 0x5e87e2 in do_video_out /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:1274:68
>>    #1 0x5df341 in reap_filters /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:1548:25
>>    #2 0x5d08b7 in transcode_from_filter /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4644:15
>>    #3 0x59e557 in transcode_step /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4729:20
>>    #4 0x593970 in transcode /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4805:15
>>    #5 0x58f7a4 in main /home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:5010:9
>>    #6 0x7f0fa9d900b2 in __libc_start_main /build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
>>    #7 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>
>> ---
>> fftools/ffmpeg.c  | 7 ++++---
>> libavutil/frame.c | 9 ++++-----
>> 2 files changed, 8 insertions(+), 8 deletions(-)
>> 
>> diff --git a/fftools/ffmpeg.c b/fftools/ffmpeg.c
>> index cfb04d5eff..cade05f762 100644
>> --- a/fftools/ffmpeg.c
>> +++ b/fftools/ffmpeg.c
>> @@ -1265,9 +1265,10 @@ static void do_video_out(OutputFile *of,
>> 
>>     if (nb0_frames == 0 && ost->last_dropped) {
>>         nb_frames_drop++;
>> -        av_log(NULL, AV_LOG_VERBOSE,
>> -               "*** dropping frame %d from stream %d at ts %"PRId64"\n",
>> -               ost->frame_number, ost->st->index, ost->last_frame->pts);
>> +        if (ost->last_frame)
>> +            av_log(NULL, AV_LOG_VERBOSE,
>> +                   "*** dropping frame %d from stream %d at ts %"PRId64"\n",
>> +                   ost->frame_number, ost->st->index, ost->last_frame->pts);
>>     }
>>     if (nb_frames > (nb0_frames && ost->last_dropped) + (nb_frames > nb0_frames)) {
>>         if (nb_frames > dts_error_threshold * 30) {
>> diff --git a/libavutil/frame.c b/libavutil/frame.c
>> index d4d3ad6988..9c866320a7 100644
>> --- a/libavutil/frame.c
>> +++ b/libavutil/frame.c
>> @@ -111,11 +111,10 @@ AVFrame *av_frame_alloc(void)
>> 
>> void av_frame_free(AVFrame **frame)
>> {
>> -    if (!frame || !*frame)
>> -        return;
>> -
>> -    av_frame_unref(*frame);
>> -    av_freep(frame);
>> +    if (*frame)
>> +        av_frame_unref(*frame);
>> +    if (frame)
>> +        av_freep(frame);
>> }
>> 
>> static int get_video_buffer(AVFrame *frame, int align)
>> 
> 
> This change to frame.c is also completely wrong; this frame should
> probably not be constantly allocated and freed and the code at lines
> 1422-1428 should actually error out in case of allocation error.
Thx, how do you think about the fix of 'ost->last_frame’?
The code at lines 1266-1270 , if ost->last_frame == NULL,
error when accessing its its.  And at lines 1422, we can know that 
ost->last_frame can be NULL. In this situation,I don’t understand that 
It is emptied and released immediately after allocation. Is it necessary?
> 
> - 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/fftools/ffmpeg.c b/fftools/ffmpeg.c
index cfb04d5eff..cade05f762 100644
--- a/fftools/ffmpeg.c
+++ b/fftools/ffmpeg.c
@@ -1265,9 +1265,10 @@  static void do_video_out(OutputFile *of,
 
     if (nb0_frames == 0 && ost->last_dropped) {
         nb_frames_drop++;
-        av_log(NULL, AV_LOG_VERBOSE,
-               "*** dropping frame %d from stream %d at ts %"PRId64"\n",
-               ost->frame_number, ost->st->index, ost->last_frame->pts);
+        if (ost->last_frame)
+            av_log(NULL, AV_LOG_VERBOSE,
+                   "*** dropping frame %d from stream %d at ts %"PRId64"\n",
+                   ost->frame_number, ost->st->index, ost->last_frame->pts);
     }
     if (nb_frames > (nb0_frames && ost->last_dropped) + (nb_frames > nb0_frames)) {
         if (nb_frames > dts_error_threshold * 30) {
diff --git a/libavutil/frame.c b/libavutil/frame.c
index d4d3ad6988..9c866320a7 100644
--- a/libavutil/frame.c
+++ b/libavutil/frame.c
@@ -111,11 +111,10 @@  AVFrame *av_frame_alloc(void)
 
 void av_frame_free(AVFrame **frame)
 {
-    if (!frame || !*frame)
-        return;
-
-    av_frame_unref(*frame);
-    av_freep(frame);
+    if (*frame)
+        av_frame_unref(*frame);
+    if (frame)
+        av_freep(frame);
 }
 
 static int get_video_buffer(AVFrame *frame, int align)