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

From: Marton Balint <cus@passwd.hu>
To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>
Subject: Re: [FFmpeg-devel] [PATCH v2] avformat/flvdec: use avio operation instead of pb->buf_ptr use
Date: Thu, 27 Jul 2023 20:56:04 +0200 (CEST)
Message-ID: <20b0c9a1-8364-21eb-10f0-1f13736a1c2@passwd.hu> (raw)
In-Reply-To: <20230727073142.64813-1-lq@chinaffmpeg.org>

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
_______________________________________________
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".