[FFmpeg-devel] [PATCH] seek: Fix crashes in ff_seek_frame_binary if built with latest Clang 14

[Martin Storsjö]

On Sat, 6 Nov 2021, Andreas Rheinhardt wrote:

> Martin Storsjö:
>> Passing an uninitialized variable as argument to a function is
>> undefined behaviour (UB). The compiler can assume that UB does not
>> happen.
>> 
>> Hence, the compiler can assume that the variables are never
>> uninitialized when passed as argument, which means that the codepaths
>> that initializes them must be taken.
>> 
>> In ff_seek_frame_binary, this means that the compiler can assume
>> that the codepaths that initialize pos_min and pos_max are taken,
>> which means that the conditions "if (sti->index_entries)" and
>> "if (index >= 0)" can be optimized out.
>> 
>> Current Clang git versions (upcoming Clang 14) enabled an optimization
>> that does this, which broke the current version of this function
>> (which intentionally left the variables uninitialized, but silencing
>> warnings about being uninitialized). See [1] for discussion on
>> the matter.
>> 
>> [1] https://reviews.llvm.org/D105169#3069555
>> 
>> Signed-off-by: Martin Storsjö <martin at martin.st>
>> ---
>>  libavformat/seek.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/libavformat/seek.c b/libavformat/seek.c
>> index 40169736df..405ca316b3 100644
>> --- a/libavformat/seek.c
>> +++ b/libavformat/seek.c
>> @@ -283,7 +283,7 @@ int ff_seek_frame_binary(AVFormatContext *s, int stream_index,
>>                           int64_t target_ts, int flags)
>>  {
>>      const AVInputFormat *const avif = s->iformat;
>> -    int64_t av_uninit(pos_min), av_uninit(pos_max), pos, pos_limit;
>> +    int64_t pos_min = 0, pos_max = 0, pos, pos_limit;
>>      int64_t ts_min, ts_max, ts;
>>      int index;
>>      int64_t ret;
>> 
>
> I already wanted to write that the compiler is wrong, but it seems it is
> not, as C11 differs from C99 in this respect (C11 6.3.2.1 2):
>
> "If the lvalue designates an
> object of automatic storage duration that could have been declared with
> the register storage class (never had its address taken), and that
> object is uninitialized (not declared
> with an initializer and no assignment to it has been performed prior to
> use), the behavior
> is undefined."
>
> For GCC and Clang av_uninit(x) is defined as x=x. And that is a problem:
> In case this macro is used to declare an automatic variable that is
> could be declared with the register storage class the
> pseudo-initialization is UB according to the above. So I think we will
> have to modify the macro to make it safe. Or just stop using it.
> (How could such a hack ever end up in a public header?)

FWIW, most of the issue here comes from the fact that the uninitialized 
value is passed as a parameter - in that respect, av_uninit() isn't any 
better or worse than just leaving the variable plain uninitialized. (Not 
that one really should reason around where UB is ok...)

Also, I haven't tried to read the standard wrt that, but I would expect 
that passing an uninitialized value as parameter was UB even before C11?

I haven't tracked the new feature upstream that closely, the change in 
clang/llvm that regressed the old code here might have been reverted 
and/or reapplied (for other reasons) though, not sure what the current 
state is.

// Martin