Re: [FFmpeg-devel] [PATCH v11 1/2] avformat/imf: Demuxer

[Pierre-Anthony Lemieux <pal@sandflow.com>]

On Fri, Dec 17, 2021 at 1:46 PM Andreas Rheinhardt
<andreas.rheinhardt@outlook.com> wrote:
>
> pal@sandflow.com:
> > From: Pierre-Anthony Lemieux <pal@palemieux.com>
> >
> > Signed-off-by: Pierre-Anthony Lemieux <pal@palemieux.com>
> > ---
> >
> > Notes:
> >     The IMF demuxer accepts as input an IMF CPL. The assets referenced by the CPL can be
> >     contained in multiple deliveries, each defined by an ASSETMAP file:
> > +    }
> > +    tmp = av_realloc(asset_map->assets,
> > +        (xmlChildElementCount(node) + asset_map->asset_count)
> > +            * sizeof(IMFAssetLocator));
>
> Can this overflow? If so, av_realloc_array() would take care of the
> overflow for the multiplication; but it would not do anything for the
> addition.

Addressed at v12. av_realloc() replaced with av_realloc_array(). Added
checks for multiplication and addition overflows before allocation.
Note: this would not overflow in usual operation, but could presumably
do so with malformed files, malicious or not.

>
> > +    if (!tmp) {
> > +        av_log(NULL, AV_LOG_ERROR, "Cannot allocate IMF asset locators\n");
> > +        return AVERROR(ENOMEM);
> > +    }
> > +static int parse_assetmap(AVFormatContext *s, const char *url)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    AVIOContext *in = NULL;
> > +    struct AVBPrint buf;
> > +    AVDictionary *opts = NULL;
>
> opts should be local to the block below (if said block is indeed needed).
>
> > +    xmlDoc *doc = NULL;
> > +    const char *base_url;
> > +    char *tmp_str = NULL;
> > +    int close_in = 0;
> > +    int ret;
> > +    int64_t filesize;
> > +
> > +    av_log(s, AV_LOG_DEBUG, "Asset Map URL: %s\n", url);
> > +
> > +    if (!in) {
>
> Is there a proper initialization for in missing above? This check is
> always true.

Addressed at v12. Removed check.

>
> > +        close_in = 1;
> > +    xmlFreeDoc(doc);
> > +
> > +clean_up:
> > +    if (tmp_str)
> > +        av_freep(&tmp_str);
> > +    if (close_in)
> > +        avio_close(in);
>
> If you open an AVIOContext via ->io_open, you should close it via
> io_close; or actually: via ff_format_io_close(s, &in).

Addressed at v12.

>
>
> > +    av_bprint_finalize(&buf, NULL);
> > +
> > +
> > +    if (track_resource->ctx->iformat) {
>
> I do not see a scenario in which track_resource->ctx could be != NULL,
> but track_resource->ctx->iformat is not: After all, you free it on error
> of ff_copy_whiteblacklists() and avformat_open_input() frees it on
> error, too. So you could just as well just check for track_resource->ctx
> at the beginning of this function and return in this case and allocate
> track_resource->ctx unconditionally in the other case.

Addressed at v12. Codepath cleaned.

>
> > +        av_log(s,
> > +            AV_LOG_DEBUG,
> > +            "Input context already opened for %s.\n",
> > +            track_resource->locator->absolute_uri);
> > +        return 0;
> > +    }
> > +
> > +    track_resource->ctx->io_open = s->io_open;
> > +    track_resource->ctx->io_close = s->io_close;
>
> We recently added an io_close2.

Addressed at v12. io_close2 added.

>
> > +    track_resource->ctx->flags |= s->flags & ~AVFMT_FLAG_CUSTOM_IO;
> > +
> > +    if ((ret = ff_copy_whiteblacklists(track_resource->ctx, s)) < 0)
> > +        goto cleanup;
> > +
> > +    av_dict_copy(&opts, c->avio_opts, 0);
> > +    ret = avformat_open_input(&track_resource->ctx,
> > +        track_resource->locator->absolute_uri,
> > +        NULL,
> > +        &opts);
> > +    av_dict_free(&opts);
> > +    if (ret < 0) {
> > +        av_log(s,
> > +            AV_LOG_ERROR,
> > +            "Could not open %s input context: %s\n",
> > +            track_resource->locator->absolute_uri,
> > +            av_err2str(ret));
> > +        return ret;
> > +    }
> > +
> > +    if (av_strcasecmp(track_resource->ctx->iformat->name, "mxf")) {
>
> strcmp -- the mxf demuxer is just called mxf. This is fixed and not
> subject to change. But see below.

Mooted by the below.

>
> > +        av_log(s,
> > +            AV_LOG_ERROR,
> > +            "Track file kind is not MXF but %s instead\n",
> > +            track_resource->ctx->iformat->name);
>
> There is a problem here: Imagine that this is called from
> imf_read_packet() via get_resource_context_for_timestamp(). Then you
> error out on that imf_read_packet() call; yet the a later
> imf_read_packet() call may see that track_resource->ctx is already set
> and just reuse it.
> The easiest fix for this is to set the ctx's format_whitelist to "mxf"
> (set this via the av_opt_set()). Alternatively, one could also set the
> ctx's iformat to the mxf demuxer unconditionally. Both of this would
> have to be done before avformat_open_input().

Addressed at v12. iformat->name check replaced with
av_opt_set(...format_whitelist...)

>
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    /* Compare the source timebase to the resource edit rate, considering the first stream of the source file */
> > +        if (ret < 0) {
> > +            av_log(s,
> > +                AV_LOG_ERROR,
> > +                "Could not seek at %" PRId64 "on %s: %s\n",
> > +                entry_point,
> > +                track_resource->locator->absolute_uri,
> > +                av_err2str(ret));
> > +            goto cleanup;
>
> resources opened with avformat_open_input() should be freed with
> avformat_close_input().

Addressed at v12.

>
> > +        }
> > +    }
> > +
> > +    return ret;
>
> return 0

Addressed at v12.

>
> > +cleanup:
> > +    avformat_free_context(track_resource->ctx);
> > +    track_resource->ctx = NULL;
> > +    return ret;
> > +}
> > +
> > +static int open_track_file_resource(AVFormatContext *s,
> > +    FFIMFTrackFileResource *track_file_resource,
> > +    IMFVirtualTrackPlaybackCtx *track)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    IMFAssetLocator *asset_locator;
> > +    IMFVirtualTrackResourcePlaybackCtx vt_ctx;
> > +    void *tmp;
> > +    int ret;
> > +
> > +    asset_locator = find_asset_map_locator(&c->asset_locator_map, track_file_resource->track_file_uuid);
> > +    if (!asset_locator) {
> > +        av_log(s,
> > +            AV_LOG_ERROR,
> > +            "Could not find asset locator for UUID: " FF_IMF_UUID_FORMAT "\n",
> > +            UID_ARG(track_file_resource->track_file_uuid));
> > +        return AVERROR_INVALIDDATA;
> > +    }
> > +
> > +    av_log(s,
> > +        AV_LOG_DEBUG,
> > +        "Found locator for " FF_IMF_UUID_FORMAT ": %s\n",
> > +        UID_ARG(asset_locator->uuid),
> > +        asset_locator->absolute_uri);
> > +    tmp = av_fast_realloc(track->resources,
> > +        &track->resources_alloc_sz,
> > +        (track->resource_count + track_file_resource->base.repeat_count)
> > +            * sizeof(IMFVirtualTrackResourcePlaybackCtx));
> > +    if (!tmp)
> > +        return AVERROR(ENOMEM);
> > +    track->resources = tmp;
> > +
> > +    for (uint32_t i = 0; i < track_file_resource->base.repeat_count; ++i) {
> > +        vt_ctx.locator = asset_locator;
> > +        vt_ctx.resource = track_file_resource;
> > +        vt_ctx.ctx = NULL;
>
> vt_ctx should be local to this loop.
>
> > +        if ((ret = open_track_resource_context(s, &vt_ctx)) != 0)
> > +            return ret;
> > +        track->resources[track->resource_count++] = vt_ctx;
> > +        track->duration = av_add_q(track->duration,
> > +            av_make_q((int)track_file_resource->base.duration * track_file_resource->base.edit_rate.den,
> > +                track_file_resource->base.edit_rate.num));
> > +    }
> > +
> > +    return ret;
>
> return 0

Addressed at v12.

>
> > +}
> > +
> > +static void imf_virtual_track_playback_context_deinit(IMFVirtualTrackPlaybackCtx *track)
> > +{
> > +    for (uint32_t i = 0; i < track->resource_count; ++i)
> > +        if (track->resources[i].ctx && track->resources[i].ctx->iformat)
> > +            avformat_close_input(&track->resources[i].ctx);
>
> avformat_close_input() can handle the case of track->resources[i].ctx
> being NULL; and given that any AVFormatContext here is either NULL or
> properly opened with avformat_open_input() you can just remove these checks.
>

Addressed at v12. Checks removed.

> > +
> > +    av_freep(&track->resources);
> > +}
> > +
> > +static int open_virtual_track(AVFormatContext *s,
> > +    FFIMFTrackFileVirtualTrack *virtual_track,
> > +    int32_t track_index)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    IMFVirtualTrackPlaybackCtx *track = NULL;
> > +    void *tmp;
> > +    int ret = 0;
> > +
> > +    if (!(track = av_mallocz(sizeof(IMFVirtualTrackPlaybackCtx))))
> > +        return AVERROR(ENOMEM);
> > +    track->index = track_index;
> > +    track->duration = av_make_q(0, 1);
> > +
> > +    for (uint32_t i = 0; i < virtual_track->resource_count; i++) {
> > +        av_log(s,
> > +            AV_LOG_DEBUG,
> > +            "Open stream from file " FF_IMF_UUID_FORMAT ", stream %d\n",
> > +            UID_ARG(virtual_track->resources[i].track_file_uuid),
> > +            i);
> > +        if ((ret = open_track_file_resource(s, &virtual_track->resources[i], track)) != 0) {
> > +            av_log(s,
> > +                AV_LOG_ERROR,
> > +                "Could not open image track resource " FF_IMF_UUID_FORMAT "\n",
> > +                UID_ARG(virtual_track->resources[i].track_file_uuid));
> > +            goto clean_up;
> > +        }
> > +    }
> > +
> > +    track->current_timestamp = av_make_q(0, track->duration.den);
> > +
> > +    tmp = av_realloc(c->tracks, (c->track_count + 1) * sizeof(IMFVirtualTrackPlaybackCtx *));
>
> Missing overflow check; av_realloc_array() would do this for you.

Addressed at v12. Replaced with av_realloc_array().

>
> > +    if (!tmp) {
> > +        ret = AVERROR(ENOMEM);
> > +        goto clean_up;
> > +    }
> > +    c->tracks = tmp;
> > +    c->tracks[c->track_count++] = track;
> > +
> > +    return 0;
> > +
> > +clean_up:
> > +    imf_virtual_track_playback_context_deinit(track);
> > +    av_free(track);
> > +    return ret;
> > +}
> > +
> > +static int set_context_streams_from_tracks(AVFormatContext *s)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +
> > +    AVStream *asset_stream;
> > +    AVStream *first_resource_stream;
>
> Both of these should be local to the loop.

Addressed at v12.

>
> > +
> > +    int ret = 0;
> > +
> > +    for (uint32_t i = 0; i < c->track_count; ++i) {
> > +        /* Open the first resource of the track to get stream information */
> > +        first_resource_stream = c->tracks[i]->resources[0].ctx->streams[0];
> > +        av_log(s, AV_LOG_DEBUG, "Open the first resource of track %d\n", c->tracks[i]->index);
> > +
> > +        /* Copy stream information */
> > +        asset_stream = avformat_new_stream(s, NULL);
> > +        if (!asset_stream) {
> > +            av_log(s, AV_LOG_ERROR, "Could not create stream\n");
>
> missing AVERROR(ENOMEM)

Addressed at v12.

>
> > +            break;
> > +        }
> > +        asset_stream->id = i;
> > +        ret = avcodec_parameters_copy(asset_stream->codecpar, first_resource_stream->codecpar);
> > +        if (ret < 0) {
> > +            av_log(s, AV_LOG_ERROR, "Could not copy stream parameters\n");
> > +            break;
>
> just return ret

Addressed at v12.

>
> > +        }
> > +        avpriv_set_pts_info(asset_stream,
> > +            first_resource_stream->pts_wrap_bits,
> > +            first_resource_stream->time_base.num,
> > +            first_resource_stream->time_base.den);
> > +        asset_stream->duration = (int64_t)av_q2d(av_mul_q(c->tracks[i]->duration,
> > +            av_inv_q(asset_stream->time_base)));
> > +    }
> > +
> > +    return ret;
>
> return 0

Addressed at v12.

>
> > +}
> > +
> > +static int save_avio_options(AVFormatContext *s)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    static const char *const opts[] = {
> > +        "headers", "http_proxy", "user_agent", "cookies", "referer", "rw_timeout", "icy", NULL};
> > +    const char *const *opt = opts;
> > +    uint8_t *buf;
> > +    int ret = 0;
> > +
> > +    char *tmp_str;
> > +    int ret = 0;
> > +
> > +    c->interrupt_callback = &s->interrupt_callback;
> > +    tmp_str = av_strdup(s->url);
> > +    if (!tmp_str) {
> > +        ret = AVERROR(ENOMEM);
>
> return AVERROR(ENOMEM);

Addressed at v12.

>
> > +        return ret;
> > +    }
> > +    c->base_url = av_dirname(tmp_str);
> > +    if ((ret = save_avio_options(s)) < 0)
> > +        return ret;
> > +
> > +    av_log(s, AV_LOG_DEBUG, "start parsing IMF CPL: %s\n", s->url);
> > +
> > +    if ((ret = ff_imf_parse_cpl(s->pb, &c->cpl)) < 0)
> > +        return ret;
> > +
> > +        }
> > +    }
> > +
> > +    av_log(s,
> > +        AV_LOG_DEBUG,
> > +        "Found next track to read: %d (timestamp: %lf / %lf)\n",
> > +        track->index,
> > +        av_q2d(track->current_timestamp),
>
> This presumes that track is set; yet isn't it possible for all tracks'
> current_timestamps to be (AVRational){ INT32_MAX, 1 }? Maybe you should
> check for <= in the above check? (If you also reverse your loop
> iterator, the result would be the same as now in the non-pathological
> case; if the order of tracks with the same current_timestamp matters at
> all).

Addressed at v12. Loop reversed and <= used.

>
> > +        av_q2d(minimum_timestamp));
> > +    return track;
> > +}
> > +
> > +static IMFVirtualTrackResourcePlaybackCtx *get_resource_context_for_timestamp(AVFormatContext *s,
> > +    IMFVirtualTrackPlaybackCtx *track)
> > +{
> > +    AVRational edit_unit_duration = av_inv_q(track->resources[0].resource->base.edit_rate);
> > +    AVRational cumulated_duration = av_make_q(0, edit_unit_duration.den);
> > +
> > +    av_log(s,
> > +        AV_LOG_DEBUG,
> > +        "Looking for track %d resource for timestamp = %lf / %lf\n",
> > +        track->index,
> > +        av_q2d(track->current_timestamp),
> > +        av_q2d(track->duration));
> > +    for (uint32_t i = 0; i < track->resource_count; ++i) {
> > +        cumulated_duration = av_add_q(cumulated_duration,
> > +            av_make_q((int)track->resources[i].resource->base.duration * edit_unit_duration.num,
> > +                edit_unit_duration.den));
> > +
> > +        if (av_cmp_q(av_add_q(track->current_timestamp, edit_unit_duration), cumulated_duration) <= 0) {
> > +            av_log(s,
> > +                AV_LOG_DEBUG,
> > +                "Found resource %d in track %d to read for timestamp %lf "
> > +                "(on cumulated=%lf): entry=%" PRIu32
> > +                ", duration=%" PRIu32
> > +                ", editrate=" AVRATIONAL_FORMAT
> > +                " | edit_unit_duration=%lf\n",
> > +                i,
> > +                track->index,
> > +                av_q2d(track->current_timestamp),
> > +                av_q2d(cumulated_duration),
> > +                track->resources[i].resource->base.entry_point,
> > +                track->resources[i].resource->base.duration,
> > +                AVRATIONAL_ARG(track->resources[i].resource->base.edit_rate),
> > +                av_q2d(edit_unit_duration));
> > +
> > +            if (track->current_resource_index != i) {
> > +                av_log(s,
> > +                    AV_LOG_DEBUG,
> > +                    "Switch resource on track %d: re-open context\n",
> > +                    track->index);
> > +                avformat_close_input(&(track->resources[track->current_resource_index].ctx));
>
> You should probably set current_resource_index to an invalid value here.
> Otherwise it might happen that if open_track_resource_context() fails,
> another call to get_resource_context_for_timestamp() (from a later
> imf_read_packet()) might return a pointer to an
> IMFVirtualTrackResourcePlaybackCtx without AVFormatContext.

Addressed at v12. The next resource is first opened, and then the
current resource is closed.

>
> > +                if (open_track_resource_context(s, &(track->resources[i])) != 0)
> > +                    return NULL;
> > +                track->current_resource_index = i;
> > +            }
> > +            return &(track->resources[track->current_resource_index]);
> > +        }
> > +    }
> > +    return NULL;
> > +}
> > +
> > +static int imf_read_packet(AVFormatContext *s, AVPacket *pkt)
> > +{
> > +    IMFContext *c = s->priv_data;
> > +    IMFVirtualTrackResourcePlaybackCtx *resource_to_read = NULL;
> > +};
> > +
> > +const AVInputFormat ff_imf_demuxer = {
> > +    .name           = "imf",
> > +    .long_name      = NULL_IF_CONFIG_SMALL("IMF (Interoperable Master Format)"),
> > +    .flags_internal = FF_FMT_INIT_CLEANUP,
> > +    .priv_class     = &imf_class,
> > +    .priv_data_size = sizeof(IMFContext),
> > +    .read_probe     = imf_probe,
> > +    .read_header    = imf_read_header,
> > +    .read_packet    = imf_read_packet,
> > +    .read_close     = imf_close
>
> Add a ','; otherwise one would have to add a ',' to this line if one
> were to add something after this line.

Addressed at v12.

>
> > +};
> >
>
> PS: Me not saying anything about imf_cpl.c is just the result of me not
> having time to look at it.

FWIW I ported your recommendations to imf_cpl.c whenever applicable.

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