Re: [FFmpeg-devel] [PATCH 3/3] Revert "avcodec/h264_mp4toannexb_bsf: fix missing PS before IDR frames"

[Zhao Zhili <quinkblack@foxmail.com>]

> On Mar 20, 2024, at 10:19, Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> This reverts commit d3aa0cd16f5e952bc346b7c74b4dcba95151a63a.
> 
> Fixes: out of array write
> Fixes: 64407/clusterfuzz-testcase-minimized-ffmpeg_BSF_H264_MP4TOANNEXB_fuzzer-4966763443650560
> 
> The bsf code performs 2 iterations, the first counts how much space is needed
> than allocates
> and the 2nd pass copies into teh allocated space
> 
> The reverted code reallocates sps/pps in the first pass in a data dependant way that leaves
> the 2nd pass in a different state then the first

Sorry for the break. How to access the fuzz report details?

Without the patch, it generates broken files in those cases. I want to dig further to fix it.

> 
> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
> libavcodec/bsf/h264_mp4toannexb.c | 83 +++----------------------------
> tests/fate/h264.mak               |  5 --
> 2 files changed, 6 insertions(+), 82 deletions(-)
> 
> diff --git a/libavcodec/bsf/h264_mp4toannexb.c b/libavcodec/bsf/h264_mp4toannexb.c
> index 120241c892..b99de39ce9 100644
> --- a/libavcodec/bsf/h264_mp4toannexb.c
> +++ b/libavcodec/bsf/h264_mp4toannexb.c
> @@ -36,8 +36,6 @@ typedef struct H264BSFContext {
>     uint8_t *pps;
>     int      sps_size;
>     int      pps_size;
> -    unsigned sps_buf_size;
> -    unsigned pps_buf_size;
>     uint8_t  length_size;
>     uint8_t  new_idr;
>     uint8_t  idr_sps_seen;
> @@ -133,33 +131,16 @@ pps:
>         memset(out + total_size, 0, padding);
> 
>     if (pps_offset) {
> -        uint8_t *sps;
> -
> +        s->sps      = out;
>         s->sps_size = pps_offset;
> -        sps = av_fast_realloc(s->sps, &s->sps_buf_size, s->sps_size);
> -        if (!sps) {
> -            av_free(out);
> -            return AVERROR(ENOMEM);
> -        }
> -        s->sps = sps;
> -        memcpy(s->sps, out, s->sps_size);
>     } else {
>         av_log(ctx, AV_LOG_WARNING,
>                "Warning: SPS NALU missing or invalid. "
>                "The resulting stream may not play.\n");
>     }
>     if (pps_offset < total_size) {
> -        uint8_t *pps;
> -
> +        s->pps      = out + pps_offset;
>         s->pps_size = total_size - pps_offset;
> -        pps = av_fast_realloc(s->pps, &s->pps_buf_size, s->pps_size);
> -        if (!pps) {
> -            av_freep(&s->sps);
> -            av_free(out);
> -            return AVERROR(ENOMEM);
> -        }
> -        s->pps = pps;
> -        memcpy(s->pps, out + pps_offset, s->pps_size);
>     } else {
>         av_log(ctx, AV_LOG_WARNING,
>                "Warning: PPS NALU missing or invalid. "
> @@ -179,35 +160,6 @@ pps:
>     return 0;
> }
> 
> -static int h264_mp4toannexb_save_ps(uint8_t **dst, int *dst_size,
> -                                    unsigned *dst_buf_size,
> -                                    const uint8_t *nal, uint32_t nal_size,
> -                                    int first)
> -{
> -    static const uint8_t nalu_header[4] = { 0, 0, 0, 1 };
> -    const int start_code_size = sizeof(nalu_header);
> -    uint8_t *ptr;
> -    uint32_t size;
> -
> -    if (first)
> -        size = 0;
> -    else
> -        size = *dst_size;
> -
> -    ptr = av_fast_realloc(*dst, dst_buf_size, size + nal_size + start_code_size);
> -    if (!ptr)
> -        return AVERROR(ENOMEM);
> -
> -    memcpy(ptr + size, nalu_header, start_code_size);
> -    size += start_code_size;
> -    memcpy(ptr + size, nal, nal_size);
> -    size += nal_size;
> -
> -    *dst = ptr;
> -    *dst_size = size;
> -    return 0;
> -}
> -
> static int h264_mp4toannexb_init(AVBSFContext *ctx)
> {
>     int extra_size = ctx->par_in->extradata_size;
> @@ -268,9 +220,6 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>     if (j) \
>         av_log(__VA_ARGS__)
>     for (int j = 0; j < 2; j++) {
> -        int sps_count = 0;
> -        int pps_count = 0;
> -
>         buf      = in->data;
>         new_idr  = s->new_idr;
>         sps_seen = s->idr_sps_seen;
> @@ -301,18 +250,8 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
> 
>             if (unit_type == H264_NAL_SPS) {
>                 sps_seen = new_idr = 1;
> -                if (!j) {
> -                    h264_mp4toannexb_save_ps(&s->sps, &s->sps_size, &s->sps_buf_size,
> -                                             buf, nal_size, !sps_count);
> -                    sps_count++;
> -                }
>             } else if (unit_type == H264_NAL_PPS) {
>                 pps_seen = new_idr = 1;
> -                if (!j) {
> -                    h264_mp4toannexb_save_ps(&s->pps, &s->pps_size, &s->pps_buf_size,
> -                                             buf, nal_size, !pps_count);
> -                    pps_count++;
> -                }
>                 /* if SPS has not been seen yet, prepend the AVCC one to PPS */
>                 if (!sps_seen) {
>                     if (!s->sps_size) {
> @@ -332,10 +271,9 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
> 
>             /* prepend only to the first type 5 NAL unit of an IDR picture, if no sps/pps are already present */
>             if (new_idr && unit_type == H264_NAL_IDR_SLICE && !sps_seen && !pps_seen) {
> -                if (s->sps_size)
> -                    count_or_copy(&out, &out_size, s->sps, s->sps_size, PS_OUT_OF_BAND, j);
> -                if (s->pps_size)
> -                    count_or_copy(&out, &out_size, s->pps, s->pps_size, PS_OUT_OF_BAND, j);
> +                if (ctx->par_out->extradata)
> +                    count_or_copy(&out, &out_size, ctx->par_out->extradata,
> +                                  ctx->par_out->extradata_size, PS_OUT_OF_BAND, j);
>                 new_idr = 0;
>             /* if only SPS has been seen, also insert PPS */
>             } else if (new_idr && unit_type == H264_NAL_IDR_SLICE && sps_seen && !pps_seen) {
> @@ -351,7 +289,7 @@ static int h264_mp4toannexb_filter(AVBSFContext *ctx, AVPacket *opkt)
>             else
>                 ps = PS_NONE;
>             count_or_copy(&out, &out_size, buf, nal_size, ps, j);
> -            if (unit_type == H264_NAL_SLICE) {
> +            if (!new_idr && unit_type == H264_NAL_SLICE) {
>                 new_idr  = 1;
>                 sps_seen = 0;
>                 pps_seen = 0;
> @@ -391,14 +329,6 @@ fail:
>     return ret;
> }
> 
> -static void h264_mp4toannexb_close(AVBSFContext *ctx)
> -{
> -    H264BSFContext *s = ctx->priv_data;
> -
> -    av_freep(&s->sps);
> -    av_freep(&s->pps);
> -}
> -
> static void h264_mp4toannexb_flush(AVBSFContext *ctx)
> {
>     H264BSFContext *s = ctx->priv_data;
> @@ -418,6 +348,5 @@ const FFBitStreamFilter ff_h264_mp4toannexb_bsf = {
>     .priv_data_size = sizeof(H264BSFContext),
>     .init           = h264_mp4toannexb_init,
>     .filter         = h264_mp4toannexb_filter,
> -    .close          = h264_mp4toannexb_close,
>     .flush          = h264_mp4toannexb_flush,
> };
> diff --git a/tests/fate/h264.mak b/tests/fate/h264.mak
> index 674054560b..d0c57eabe9 100644
> --- a/tests/fate/h264.mak
> +++ b/tests/fate/h264.mak
> @@ -227,7 +227,6 @@ FATE_H264-$(call FRAMECRC, MOV, H264) += fate-h264-twofields-packet
> FATE_H264-$(call DEMMUX, MOV, H264, H264_MP4TOANNEXB_BSF SCALE_FILTER) += fate-h264-bsf-mp4toannexb-new-extradata
> 
> FATE_H264-$(call DEMMUX, MOV, H264, H264_MP4TOANNEXB_BSF) += fate-h264-bsf-mp4toannexb \
> -                                                             fate-h264-bsf-mp4toannexb-2 \
>                                                              fate-h264_mp4toannexb_ticket5927 \
>                                                              fate-h264_mp4toannexb_ticket5927_2 \
> 
> @@ -432,10 +431,6 @@ fate-h264-conformance-sva_nl1_b:                  CMD = framecrc -i $(TARGET_SAM
> fate-h264-conformance-sva_nl2_e:                  CMD = framecrc -i $(TARGET_SAMPLES)/h264-conformance/SVA_NL2_E.264
> 
> fate-h264-bsf-mp4toannexb:                        CMD = md5 -i $(TARGET_SAMPLES)/h264/interlaced_crop.mp4 -c:v copy -f h264
> -# First IDR is prefixed by SPS/PPS
> -fate-h264-bsf-mp4toannexb-2:                      CMD = md5 -i $(TARGET_SAMPLES)/h264/ps_prefix_first_idr.mp4 -c:v copy -f h264
> -fate-h264-bsf-mp4toannexb-2:                      CMP = oneline
> -fate-h264-bsf-mp4toannexb-2:                      REF = cffcfa6a2d0b58c9de1f5785f099f41d
> fate-h264-bsf-mp4toannexb-new-extradata:          CMD = stream_remux mov $(TARGET_SAMPLES)/h264/extradata-reload-multi-stsd.mov "" h264 "-map 0:v"
> fate-h264_mp4toannexb_ticket5927:                 CMD = transcode "mp4" $(TARGET_SAMPLES)/h264/thezerotheorem-cut.mp4 \
>                                                         h264 "-c:v copy -bsf:v h264_mp4toannexb -an" "-c:v copy"
> -- 
> 2.17.1
> 
> _______________________________________________
> 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".

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