diff mbox series

[FFmpeg-devel,v2] avformat/flvdec: use avio operation instead of pb->buf_ptr use

Message ID 20230727073142.64813-1-lq@chinaffmpeg.org
State New
Headers show
Series [FFmpeg-devel,v2] avformat/flvdec: use avio operation instead of pb->buf_ptr use | expand

Checks

Context Check Description
yinshiyou/make_loongarch64 success Make finished
yinshiyou/make_fate_loongarch64 success Make fate finished
andriy/make_x86 success Make finished
andriy/make_fate_x86 success Make fate finished

Commit Message

Liu Steven July 27, 2023, 7:31 a.m. UTC
check ensure seekback 4 bytes before read 4 bytes from pb,
and seek back 4 byte from current position after read 4 bytes.

fix segfaults:
READ of size 1 at 0x6100000003b7 thread T0
    #0 0x7f928d in flv_same_video_codec ffmpeg/libavformat/flvdec.c:317:29
    #1 0x7f928d in flv_read_packet ffmpeg/libavformat/flvdec.c:1177
    #2 0x6ff32f in ff_read_packet ffmpeg/libavformat/demux.c:575:15
    #3 0x70a2fd in read_frame_internal ffmpeg/libavformat/demux.c:1263:15
    #4 0x71d158 in avformat_find_stream_info ffmpeg/libavformat/demux.c:2634:15
    #5 0x4c821b in LLVMFuzzerTestOneInput ffmpeg/tools/target_dem_fuzzer.c:206:11

Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
---
 libavformat/flvdec.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Marton Balint July 27, 2023, 6:56 p.m. UTC | #1
On Thu, 27 Jul 2023, Steven Liu wrote:

> check ensure seekback 4 bytes before read 4 bytes from pb,
> and seek back 4 byte from current position after read 4 bytes.
>
> fix segfaults:
> READ of size 1 at 0x6100000003b7 thread T0
>    #0 0x7f928d in flv_same_video_codec ffmpeg/libavformat/flvdec.c:317:29
>    #1 0x7f928d in flv_read_packet ffmpeg/libavformat/flvdec.c:1177
>    #2 0x6ff32f in ff_read_packet ffmpeg/libavformat/demux.c:575:15
>    #3 0x70a2fd in read_frame_internal ffmpeg/libavformat/demux.c:1263:15
>    #4 0x71d158 in avformat_find_stream_info ffmpeg/libavformat/demux.c:2634:15
>    #5 0x4c821b in LLVMFuzzerTestOneInput ffmpeg/tools/target_dem_fuzzer.c:206:11
>
> Signed-off-by: Steven Liu <lq@chinaffmpeg.org>
> ---
> libavformat/flvdec.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
> index 3fe21622f7..38f34567dd 100644
> --- a/libavformat/flvdec.c
> +++ b/libavformat/flvdec.c
> @@ -35,6 +35,7 @@
> #include "libavutil/intreadwrite.h"
> #include "libavutil/mathematics.h"
> #include "avformat.h"
> +#include "avio_internal.h"
> #include "demux.h"
> #include "internal.h"
> #include "flv.h"
> @@ -313,8 +314,14 @@ static int flv_same_video_codec(AVFormatContext *s, AVCodecParameters *vpar, int
>         return 1;
>
>     if (flv->exheader) {
> -        uint8_t *codec_id_str = (uint8_t *)s->pb->buf_ptr;
> -        uint32_t codec_id = codec_id_str[3] | codec_id_str[2] << 8 | codec_id_str[1] << 16 | codec_id_str[0] << 24;
> +        uint32_t codec_id = 0;
> +        int ret = ffio_ensure_seekback(s->pb, 4);
> +        if (ret < 0) {
> +            av_log(s, AV_LOG_WARNING, "Could not ensure seekback, because %s", av_err2str(ret));
> +            return 0;
> +        }
> +        codec_id = avio_rb32(s->pb);
> +        avio_seek(s->pb, -4, SEEK_CUR);

Can't you rework the code to not do any IO here? It is super confusing 
that a function called "flv_same_video_codec" actually reads stuff instead 
of using only its parameters to check it.

IMHO the fourcc should be read where VideoTagHeader is read, not here. And 
the fourcc should be passed as parameter to flv_same_video_codec. Or maybe 
you can use a new variable called videocodecid, and set it to either 
fourcc or legacy videocodecid, and pass only that to flv_same_video_codec.

Regards,
Marton
diff mbox series

Patch

diff --git a/libavformat/flvdec.c b/libavformat/flvdec.c
index 3fe21622f7..38f34567dd 100644
--- a/libavformat/flvdec.c
+++ b/libavformat/flvdec.c
@@ -35,6 +35,7 @@ 
 #include "libavutil/intreadwrite.h"
 #include "libavutil/mathematics.h"
 #include "avformat.h"
+#include "avio_internal.h"
 #include "demux.h"
 #include "internal.h"
 #include "flv.h"
@@ -313,8 +314,14 @@  static int flv_same_video_codec(AVFormatContext *s, AVCodecParameters *vpar, int
         return 1;
 
     if (flv->exheader) {
-        uint8_t *codec_id_str = (uint8_t *)s->pb->buf_ptr;
-        uint32_t codec_id = codec_id_str[3] | codec_id_str[2] << 8 | codec_id_str[1] << 16 | codec_id_str[0] << 24;
+        uint32_t codec_id = 0;
+        int ret = ffio_ensure_seekback(s->pb, 4);
+        if (ret < 0) {
+            av_log(s, AV_LOG_WARNING, "Could not ensure seekback, because %s", av_err2str(ret));
+            return 0;
+        }
+        codec_id = avio_rb32(s->pb);
+        avio_seek(s->pb, -4, SEEK_CUR);
         switch(codec_id) {
             case MKBETAG('h', 'v', 'c', '1'):
                 return vpar->codec_id == AV_CODEC_ID_HEVC;