Re: mpv

On Sun, Dec 17, 2023 at 04:38:02PM +0100, Theo Buehler wrote:
> > mpv crashes after seeking forward(->) any .opus file on obsd current
>
> Yes, any seek crashes mpv and mplayer. It's a bug in ffmpeg's seek code
> in libavformat and the trace always looks the same:
>
> #0 ff_seek_frame_binary (s=0xfa08e08a800, stream_index=0, target_ts=<optimized out>, flags=1)
> at src/libavformat/utils.c:2190
> #1 0x00000fa126e2475c in ogg_read_seek (s=0xfa08e08a800, stream_index=0, timestamp=303408,
> flags=1) at src/libavformat/oggdec.c:954
> #2 0x00000fa126e8c942 in seek_frame_internal (s=0xfa08e08a800, stream_index=0, timestamp=303408,
> flags=1) at src/libavformat/utils.c:2472
> #3 av_seek_frame (s=0xfa08e08a800, stream_index=<optimized out>, timestamp=<optimized out>,
> flags=1) at src/libavformat/utils.c:2504
>
> It's been present since the switch to llvm 16, presumably some more
> aggressive optimization around undefined behavior. I haven't spotted
> the precise issue, but the below diff works around the crash:

This seems to come from the use of the av_uninit() macro:

# define av_uninit(x) x=x

"av_uninit() to suppress false uninitialized warnings from gcc without deoptimizing code.
https://github.com/FFmpeg/FFmpeg/commit/51066987cf05a7cad567e965fa637e28f9d902c5

This seems a dubious idea to begin with, even 15 years ago, but with
the aggressive optimizations of modern compilers it's downright dangerous.

int64_t av_uninit(pos_min), av_uninit(pos_max), pos, pos_limit;
...
index = av_index_search_timestamp(st, target_ts,
flags & ~AVSEEK_FLAG_BACKWARD);
av_assert0(index < st->nb_index_entries);
if (index >= 0) {
e = &st->index_entries[index];
av_assert1(e->timestamp >= target_ts);
pos_max = e->pos;

The crash happens in the above line, which is the only spot initializing
pos_max.

...
}

pos = ff_gen_search(s, stream_index, target_ts, pos_min, pos_max, pos_limit,
ts_min, ts_max, flags, &ts, avif->read_timestamp);

But here pos_max is passed by value to ff_gen_search. Passing by value
implies reading the value, which implies the value must be well-defined
which in turn means that index >= 0 always. Or something like that.

In any case, the compiler elides the index >= 0 check in unmodified code
and leaves it there when disabling optimization, or alternatively when
initializing pos_max:

--- libavformat/utils.c.orig
+++ libavformat/utils.c
@@ -2146,7 +2146,7 @@ int ff_seek_frame_binary(AVFormatContext *s, int strea
int64_t target_ts, int flags)
{
const AVInputFormat *avif = s->iformat;
- int64_t av_uninit(pos_min), av_uninit(pos_max), pos, pos_limit;
+ int64_t av_uninit(pos_min), pos_max = 0, pos, pos_limit;
int64_t ts_min, ts_max, ts;
int index;
int64_t ret;