On Fri, Dec 17, 2021 at 1:46 PM Andreas Rheinhardt
<andreas.rheinha...@outlook.com> wrote:
>
> p...@sandflow.com:
> > From: Pierre-Anthony Lemieux <p...@palemieux.com>
> >
> > Signed-off-by: Pierre-Anthony Lemieux <p...@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-requ...@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-requ...@ffmpeg.org with subject "unsubscribe".
