On 31.10.2016 09:51, Stefano Sabatini wrote:
> From 7f209e27aa33e8f43444e5cfc44c68f664b69e06 Mon Sep 17 00:00:00 2001
> From: Nicolas George <geo...@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
> <stefa...@gmail.com>.
>
> Signed-off-by: Nicolas George <geo...@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
>
> +@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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
