[FFmpeg-devel] [PATCH] lavf: add ffprobe demuxer

Andreas Cadhalpun andreas.cadhalpun at googlemail.com
Sun Dec 4 23:54:21 EET 2016


On 31.10.2016 09:51, Stefano Sabatini wrote:
> From 7f209e27aa33e8f43444e5cfc44c68f664b69e06 Mon Sep 17 00:00:00 2001
> From: Nicolas George <george at nsup.org>
> Date: Sat, 11 Jan 2014 19:42:41 +0100
> Subject: [PATCH] lavf: add ffprobe demuxer
> 
> With several modifications and documentation by Stefano Sabatini
> <stefasab at gmail.com>.
> 
> Signed-off-by: Nicolas George <george at nsup.org>
> ---
>  configure                |   3 +
>  doc/demuxers.texi        |  18 ++
>  doc/ffprobe-format.texi  | 121 +++++++++++++
>  doc/formats.texi         |   1 +
>  libavformat/Makefile     |   1 +
>  libavformat/allformats.c |   1 +
>  libavformat/ffprobedec.c | 439 +++++++++++++++++++++++++++++++++++++++++++++++
>  7 files changed, 584 insertions(+)
>  create mode 100644 doc/ffprobe-format.texi
>  create mode 100644 libavformat/ffprobedec.c
> 
> diff --git a/configure b/configure
> index 72ffaea..71b9d73 100755
> --- a/configure
> +++ b/configure
> @@ -3349,6 +3349,9 @@ for n in $COMPONENT_LIST; do
>      eval ${n}_if_any="\$$v"
>  done
>  
> +# Disable ffprobe demuxer for security concerns
> +disable ffprobe_demuxer
> +

As I already wrote elsewhere, I don't think disabling this by default is good,
as it will likely cause it to bitrot. Better require '-strict experimental'.

>  enable $ARCH_EXT_LIST
>  
>  die_unknown(){
> diff --git a/doc/demuxers.texi b/doc/demuxers.texi
> index 2934a1c..0e6dfbd 100644
> --- a/doc/demuxers.texi
> +++ b/doc/demuxers.texi
> @@ -243,6 +243,24 @@ file subdir/file-2.wav
>  @end example
>  @end itemize
>  
> + at section ffprobe
> +
> +ffprobe internal demuxer. It is able to demux files in the format
> +specified in the @ref{FFprobe demuxer format} chapter.
> +
> +For security reasons this demuxer is disabled by default, should be
> +enabled though the @code{--enable-demuxer=ffprobe} configure option.
> +

In that case the documentation should mention '-strict experimental'
instead of this.

> diff --git a/libavformat/ffprobedec.c b/libavformat/ffprobedec.c
> new file mode 100644
> index 0000000..0bc9a49
> --- /dev/null
> +++ b/libavformat/ffprobedec.c
[...]
> +static int read_section_stream(AVFormatContext *avf)
> +{
> +    FFprobeContext *ffp = avf->priv_data;
> +    uint8_t buf[LINE_BUFFER_SIZE];
> +    int ret, index, val1, val2;
> +    AVStream *st = NULL;
> +    const char *val;
> +
> +    av_bprint_clear(&ffp->data);
> +    while ((ret = read_section_line(avf, buf, sizeof(buf)))) {
> +        if (ret < 0)
> +            return ret;
> +        if (!st) {
> +            if (sscanf(buf, "index=%d", &index) >= 1) {
> +                if (index == avf->nb_streams) {
> +                    if (!avformat_new_stream(avf, NULL))
> +                        return AVERROR(ENOMEM);
> +                }
> +                if ((unsigned)index >= avf->nb_streams) {
> +                    av_log(avf, AV_LOG_ERROR, "Invalid stream index: %d\n",
> +                           index);
> +                    return AVERROR_INVALIDDATA;
> +                }
> +                st = avf->streams[index];
> +            } else {
> +                av_log(avf, AV_LOG_ERROR, "Stream without index\n");
> +                return AVERROR_INVALIDDATA;
> +            }
> +        }
> +        if (av_strstart(buf, "codec_name=", &val)) {
> +            const AVCodecDescriptor *desc = avcodec_descriptor_get_by_name(val);
> +            if (desc) {
> +                st->codecpar->codec_id   = desc->id;
> +                st->codecpar->codec_type = desc->type;
> +            }
> +            if (!desc) {
> +                av_log(avf, AV_LOG_WARNING, "Cannot recognize codec name '%s'", val);
> +            }
> +        } else if (!strcmp(buf, "extradata=")) {
> +            if ((ret = read_data(avf, NULL)) < 0)
> +                return ret;
> +            if (ffp->data.len) {

This can leak st->codecpar->extradata and thus needs:
                   av_freep(&st->codecpar->extradata);

> +                if ((ret = ff_alloc_extradata(st->codecpar, ffp->data.len)) < 0)
> +                    return ret;
> +                memcpy(st->codecpar->extradata, ffp->data.str, ffp->data.len);
> +            }
> +        } else if (sscanf(buf, "time_base=%d/%d", &val1, &val2) >= 2) {
> +            st->time_base.num = val1;
> +            st->time_base.den = val2;
> +            if (st->time_base.num <= 0 || st->time_base.den <= 0) {

This should check val1 and val2 and only afterwards set st->time_base.
Otherwise the check doesn't have the desired effect.

> +                av_log(avf, AV_LOG_ERROR, "Invalid time base %d/%d\n",
> +                       st->time_base.num, st->time_base.den);
> +                return AVERROR_INVALIDDATA;
> +            }
> +        }
> +    }
> +    return SEC_STREAM;
> +}
> +
> +static int read_section_packet(AVFormatContext *avf, AVPacket *pkt)
> +{
> +    FFprobeContext *ffp = avf->priv_data;
> +    uint8_t buf[LINE_BUFFER_SIZE];
> +    int ret;
> +    AVPacket p;
> +    char flags;
> +    int has_stream_index = 0;
> +    int64_t pts, dts, duration;

These three variables need to be initialized to prevent use of uninitialized memory.

> +
> +    av_init_packet(&p);
> +    p.pos = avio_tell(avf->pb);
> +    p.stream_index = -1;
> +    p.size = -1;
> +    av_bprint_clear(&ffp->data);
> +
> +    while ((ret = read_section_line(avf, buf, sizeof(buf)))) {
> +        int has_time = 0;
> +        char timebuf[LINE_BUFFER_SIZE];

This buffer needs to be initialized, as well.

> +
> +        if (ret < 0)
> +            return ret;
> +        if (sscanf(buf, "stream_index=%d", &p.stream_index)) {
> +            if ((unsigned)p.stream_index >= avf->nb_streams) {
> +                av_log(avf, AV_LOG_ERROR, "Invalid stream number %d specified in packet number %d\n",
> +                       p.stream_index, ffp->packet_nb);
> +                return AVERROR_INVALIDDATA;
> +            }
> +            has_stream_index = 1;
> +        }
> +
> +#define PARSE_TIME(name_, is_duration)                                  \
> +        sscanf(buf, #name_ "=%"SCNi64, &p.name_);                       \
> +        has_time = sscanf(buf, #name_ "_time=%s", timebuf);             \
> +        if (has_time) {                                                 \
> +            if (!strcmp(timebuf, "N/A")) {                              \
> +                p.name_ = is_duration ? 0 : AV_NOPTS_VALUE;             \
> +            } else {                                                    \
> +                ret = av_parse_time(&name_, timebuf, 1);                \
> +                if (ret < 0) {                                          \
> +                    av_log(avf, AV_LOG_ERROR, "Invalid " #name_ " time specification '%s' for packet #%d data\n", \
> +                           timebuf, ffp->packet_nb);                    \
> +                    return ret;                                         \
> +                }                                                       \
> +            }                                                           \
> +        }                                                               \
> +
> +        PARSE_TIME(pts, 0);
> +        PARSE_TIME(dts, 0);
> +        PARSE_TIME(duration, 1);
> +
> +        if (sscanf(buf, "flags=%c", &flags) >= 1)
> +            p.flags = flags == 'K' ? AV_PKT_FLAG_KEY : 0;
> +        if (!strcmp(buf, "data="))
> +            if ((ret = read_data(avf, &p.size)) < 0)
> +                return ret;
> +    }
> +
> +    if (!has_stream_index) {
> +        av_log(avf, AV_LOG_ERROR, "No stream index was specified for packet #%d, aborting\n", ffp->packet_nb);
> +        return AVERROR_INVALIDDATA;
> +    }
> +
> +    if (p.size < 0 || (unsigned)p.stream_index >= avf->nb_streams)
> +        return SEC_NONE;
> +
> +    p.pts = av_rescale_q(pts, AV_TIME_BASE_Q, avf->streams[p.stream_index]->time_base);
> +    p.dts = av_rescale_q(dts, AV_TIME_BASE_Q, avf->streams[p.stream_index]->time_base);
> +    p.duration = av_rescale_q(duration, AV_TIME_BASE_Q, avf->streams[p.stream_index]->time_base);
> +
> +    if ((ret = av_new_packet(pkt, p.size)) < 0)
> +        return ret;
> +    p.data = pkt->data;
> +    p.buf  = pkt->buf;
> +    *pkt = p;
> +    if (ffp->data.len) {
> +        ffp->data.len = FFMIN(ffp->data.len, pkt->size);
> +        memcpy(pkt->data, ffp->data.str, ffp->data.len);
> +    }
> +    return SEC_PACKET;
> +}
[...]
> +static int ffprobe_read_header(AVFormatContext *avf)
> +{
> +    FFprobeContext *ffp = avf->priv_data;
> +    int ret;
> +    int nb_streams = 0;
> +

I suggest to add here something like:
    if (avf->strict_std_compliance > FF_COMPLIANCE_EXPERIMENTAL) {
        av_log(avf, AV_LOG_ERROR, "The ffprobe demuxer is experimental and requires '-strict experimental'.\n");
        return AVERROR_EXPERIMENTAL;
    }

Otherwise this patch looks good to me.

Best regards,
Andreas



More information about the ffmpeg-devel mailing list