diff mbox series

[FFmpeg-devel] fftools/ffmpeg_filter: fix SEGV in choose_pix_fmts after avio_close_dyn_buf

Message ID 20211201083752.66991-1-lq@chinaffmpeg.org
State New
Headers show
Series [FFmpeg-devel] fftools/ffmpeg_filter: fix SEGV in choose_pix_fmts after avio_close_dyn_buf
Related show

Checks

Context Check Description
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished
andriy/make_ppc success Make finished
andriy/make_fate_ppc success Make fate finished

Commit Message

Steven Liu Dec. 1, 2021, 8:37 a.m. UTC
From: Steven Liu <liuqi05@kuaishou.com>

ret could be set to s->opaque->buffer in avio_close_dyn_buf, so it can
be set to NULL, check NULL pointer deference after it should be ok.

Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
Signed-off-by: Steven Liu <liuqi05@kuaishou.com>
---
 fftools/ffmpeg_filter.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Anton Khirnov Dec. 1, 2021, 10:20 a.m. UTC | #1
Quoting Steven Liu (2021-12-01 09:37:52)
> From: Steven Liu <liuqi05@kuaishou.com>
> 
> ret could be set to s->opaque->buffer in avio_close_dyn_buf, so it can
> be set to NULL, check NULL pointer deference after it should be ok.

I don't understand this reasoning. avio_close_dyn_buf() should produce
a non-NULL output buffer if something was written into it. Since this
branch is only taken when (enc->pix_fmts != NULL), something should
always be written, so the output buffer should be non-NULL and have a
non-zero size.

Under what circumstances will it happen that the output is NULL?
Steven Liu Dec. 1, 2021, 10:27 a.m. UTC | #2
Anton Khirnov <anton@khirnov.net> 于2021年12月1日周三 下午6:21写道:
>
> Quoting Steven Liu (2021-12-01 09:37:52)
> > From: Steven Liu <liuqi05@kuaishou.com>
> >
> > ret could be set to s->opaque->buffer in avio_close_dyn_buf, so it can
> > be set to NULL, check NULL pointer deference after it should be ok.
>
> I don't understand this reasoning. avio_close_dyn_buf() should produce
> a non-NULL output buffer if something was written into it. Since this
> branch is only taken when (enc->pix_fmts != NULL), something should
> always be written, so the output buffer should be non-NULL and have a
> non-zero size.
>
> Under what circumstances will it happen that the output is NULL?

Hi Anton,

This is the message:

ffmpeg version 4.4.1 Copyright (c) 2000-2021 the FFmpeg developers
  built with clang version 10.0.0-4ubuntu1
  configuration: --enable-gpl --enable-nonfree --disable-doc
--disable-stripping --disable-asm --disable-optimizations
--cc=/home/r1/src_git/mfuzz-project/build/bin/mclangwrapper
--cxx=/home/r1/src_git/mfuzz-project/build/bin/mclangwrapper++
  libavutil      56. 70.100 / 56. 70.100
  libavcodec     58.134.100 / 58.134.100
  libavformat    58. 76.100 / 58. 76.100
  libavdevice    58. 13.100 / 58. 13.100
  libavfilter     7.110.100 /  7.110.100
  libswscale      5.  9.100 /  5.  9.100
  libswresample   3.  9.100 /  3.  9.100
  libpostproc    55.  9.100 / 55.  9.100
Input #0, mov,mp4,m4a,3gp,3g2,mj2, from './file':
  Metadata:
    major_brand     : isom
    minor_version   : 512
    compatible_brands: isomiso2mp41
    encoder         : Lavf58.76.100
  Duration: 00:00:02.00, start: 0.000000, bitrate: 113 kb/s
  Stream #0:0(und): Video: mpeg4 (Simple Profile) (mp4v / 0x7634706D),
yuv420p(tv, bt709), 640x480 [SAR 6:5 DAR 8:5], 109 kb/s, 3 fps, 3 tbr,
12288 tbn, 3 tbc (default)
    Metadata:
      handler_name    : VideoHandler
      vendor_id       : [0][0][0][0]
Stream mapping:
  Stream #0:0 -> #0:0 (mpeg4 (native) -> mpeg4 (native))
Press [q] to stop, [?] for help
faultinject: error actived 1! name: realloc, file:
src/libavutil/mem.c, line: 142
??:?
/home/r1/src_git/mfuzz-project/wrapper/mfuzzrt/faultinject.c:44
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavutil/mem.c:142
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavutil/mem.c:171
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavformat/aviobuf.c:1312
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavformat/aviobuf.c:170
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavformat/aviobuf.c:191
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavformat/aviobuf.c:248
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/libavformat/aviobuf.c:1442
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_filter.c:129
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_filter.c:476
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_filter.c:657
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_filter.c:1076
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:2237
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:2318
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:2515
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:2675
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4396
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4751
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4805
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:5010
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf3)[0x7ff6ec5900b3]
??:?
AddressSanitizer:DEADLYSIGNAL
=================================================================
==2784535==ERROR: AddressSanitizer: SEGV on unknown address (pc
0x00000055552e bp 0x7fffaec75a90 sp 0x7fffaec75780 T0)
==2784535==The signal is caused by a READ memory access.
==2784535==Hint: this fault was caused by a dereference of a high
value address (see register values below).  Dissassemble the provided
pc to learn which register was used.
    #0 0x55552e in choose_pix_fmts
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_filter.c:130:22
    #1 0x54f58a in configure_output_video_filter
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_filter.c:476:21
    #2 0x5442b0 in configure_output_filter
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_filter.c:657:37
    #3 0x541012 in configure_filtergraph
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_filter.c:1076:9
    #4 0x6033de in ifilter_send_frame
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:2237:15
    #5 0x6018e3 in send_frame_to_filters
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:2318:15
    #6 0x5fa778 in decode_video
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:2515:11
    #7 0x5a5c8b in process_input_packet
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:2675:19
    #8 0x5d281e in process_input
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4396:23
    #9 0x59f1cf in transcode_step
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4751:11
    #10 0x593970 in transcode
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:4805:15
    #11 0x58f7a4 in main
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg.c:5010:9
    #12 0x7ff6ec5900b2 in __libc_start_main
/build/glibc-eX1tMB/glibc-2.31/csu/../csu/libc-start.c:308:16
    #13 0x42033d in _start (/home/r1/ffmpeg/ffmpeg_4.4.1+0x42033d)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV
/home/r1/ffmpeg/ffmpeg-4.4.1/build/src/fftools/ffmpeg_filter.c:130:22
in choose_pix_fmts




```c
// in libavformat/aviobuf.c:1294
1294 static int dyn_buf_write(void *opaque, uint8_t *buf, int buf_size)
1295 {
...
// av_reallocp() could set `opaque->buffer = NULL`; see below;
1312 if ((err = av_reallocp(&d->buffer, new_allocated_size)) < 0) {
...
1316 }
...
1324 }

```
```c
// in libavutil/mem.c:161
161 int av_reallocp(void *ptr, size_t size)
162 {
...
171 val = av_realloc(val, size);
172
173 if (!val) {
// av_freep() could set `ptr = NULL`;
174 av_freep(ptr);
175 return AVERROR(ENOMEM);
176 }
...
180 }

```

```c
// in fftools/ffmpeg_filter.c:94
94 static char *choose_pix_fmts(OutputFilter *ofilter)
95 {
...
114 uint8_t *ret;
...
// `avio_close_dyn_buf()` set `ret = s->opaque->buffer`, so ret may be NULL;
129 len = avio_close_dyn_buf(s, &ret);
// NULL pointer deference.
130 ret[len - 1] = 0;
...
134 }

```

Thanks
Steven
Anton Khirnov Dec. 1, 2021, 10:40 a.m. UTC | #3
Quoting Steven Liu (2021-12-01 11:27:41)
> Anton Khirnov <anton@khirnov.net> 于2021年12月1日周三 下午6:21写道:
> >
> > Quoting Steven Liu (2021-12-01 09:37:52)
> > > From: Steven Liu <liuqi05@kuaishou.com>
> > >
> > > ret could be set to s->opaque->buffer in avio_close_dyn_buf, so it can
> > > be set to NULL, check NULL pointer deference after it should be ok.
> >
> > I don't understand this reasoning. avio_close_dyn_buf() should produce
> > a non-NULL output buffer if something was written into it. Since this
> > branch is only taken when (enc->pix_fmts != NULL), something should
> > always be written, so the output buffer should be non-NULL and have a
> > non-zero size.
> >
> > Under what circumstances will it happen that the output is NULL?
> 
> ```c
> // in libavformat/aviobuf.c:1294
> 1294 static int dyn_buf_write(void *opaque, uint8_t *buf, int buf_size)
> 1295 {
> ...
> // av_reallocp() could set `opaque->buffer = NULL`; see below;
> 1312 if ((err = av_reallocp(&d->buffer, new_allocated_size)) < 0) {

So the problem is that memory allocation fails and nothing gets written
in the buffer?

Then it seems more correct to check the return value of avio_printf().
diff mbox series

Patch

diff --git a/fftools/ffmpeg_filter.c b/fftools/ffmpeg_filter.c
index 452b689d62..f2f77ff86f 100644
--- a/fftools/ffmpeg_filter.c
+++ b/fftools/ffmpeg_filter.c
@@ -119,6 +119,8 @@  static char *choose_pix_fmts(OutputFilter *ofilter)
             avio_printf(s, "%s|", name);
         }
         len = avio_close_dyn_buf(s, &ret);
+        if (len <= 0 || !ret)
+            return NULL;
         ret[len - 1] = 0;
         return ret;
     } else