[MPlayer-dev-eng] demux_mf: improve format string processing

[Alexander Strasser]

Hi all!

@Compn: Thanks for forwarding.

Many thanks to the mpv folks for letting us know about this.
avih and jeeb are Bcc'ed on this mail. Maybe you are interested in our
take on this or further collaboration. There is also a second patch
for an entirely different problem linked below, that might be
applicable to mpv in demux_mf.c : probe_format .

On 2021-04-09 12:10 -0700, Patriot ACT wrote:
> Forgive me, not in a patch format... copied from irc:
>
> <JEEB> since this code originates in mplayer and I wasn't able to find
> an up-to-date web-viewable SVN repo, this might be applicable to
> mplayer as well
>
> https://github.com/mpv-player/mpv/commit/d0c530919d8cd4d7a774e38ab064e0fabdae34e6

That code is still in MPlayer.

After some discussion with Reimar, we concluded that mostly everything
is lost if the attacker can control the command line or more in general
the arguments passed to the mplayer process.

The sprintf is bad anyway, so I painfully came up with a patch to
replace it. While doing that I promptly stumbled over another bug.
Both patches sent here in this thread:
  http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/2021-April/073973.html

1. libmpdemux/mf: Replace sprintf by mp_asprintf
2. libmpdemux/demux_mf: Don't crash if no paths were selected

Starting to wonder how exactly this functionality came to happen and
evolved, I did a little code archaeology.

The core functionality is from 2 commits at 6th and 7th February of 2002.

That is after I started using MPlayer around 2001, but years before I
started reading or even working on MPlayer's code base.

On the first day it included support for globbing and on its second
day, support for printf format strings was added.

Over the years mf:// was expanded.

In March 2002 support for "comma delimited filenames" was added.

In November 2005 support for using a text file containing the file names
one per line was added.

In all those years I think the format of the URL was never documented.
That part the man page describes as `filemask|@listfile`.

In MEncoder's HTML docs some amount of documentation-by-sample
command lines can be found.

Finding out this wasn't documented for almost 20 year, I actually
think it would make sense to have this documentation and to rethink
what it should look like. If we can come up with a good and mostly
backward compatible sane documentation for this feature I would
like to see it reimplemented from scratch.

Not sure when or if it will ever happen, but as a first step in
that direction I will start with a documentation for the current
state of mf:

---8<---

    mf://[<mf spec>]

The *mf spec* is a specification from which the input paths should be
deduced. It can be one of these:

    <glob pattern>|<printf format>|<comma delimited paths>|@<list file>

If the *mf spec* is left out, a shell globbing pattern of `*` is used.

## list file

If the first character of after *mf spec* is `@`, the remaining
characters will be used as a path to open as a file. If the file is
readable, each line will be interpreted as one path for the multi
file input.

## comma delimited paths

If *mf spec* contains a `,` it is considered to be list of comma
delimited paths, that will be used as input paths in that order.

## glob pattern

If it **doesn't** contain a `%` it is assumed to be a glob pattern.
The glob pattern will be evaluated and the resulting list of files
if any, will be used as paths for the multi file input.

If the glob pattern doesn't contain a `*`, it will be appended.

## printf format

If *mf spec* **didn't** yet match any of the cases described above,
it will be considered to be a printf format string. This string
will be used to render an input path with a single int argument.
This argument is a counter starting at zero, incrementing by one
after each call.

Should it happen that for, in sum, 5 of any of the rendered paths
the file status (stat) couldn't be retrieved, the path sequence
will be considered finished.

--->8---

WARNING: I reverse engineered the description from the source
code, but didn't actually test if all my assertions are correct.


Best regards,
  Alexander