Hello,
1. I agree that av_uninit should be removed; in fact, I also wanted to
do so and already made some commits to do so where I consider the commit
to be beneficial on its own (even without the av_uninit removal) like
348461e550f88c5e27afeafc85e7ace11fe8fae4 and its parent.
2. You fail to actually cite the spec. Here is the proper part (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."
According to this, most of the uses of av_uninit are UB themselves, so
removing it is really fixing UB. This makes Nicolas' counterpoint about
hiding UB moot. It is also the reason why I wanted to remove av_uninit.
3. But I disagree that the variables marked with av_uninit should be
initialized. They need not be unless the initial value is really used
for something (even if only passed to a function, as this would be UB
according to the spec cited above; this is the reason behind
ab792634197e364ca1bb194f9abe36836e42f12d, whereas tools like Valgrind
where happy to pass an uninitialized value to a function if said
function would not use it). Just like we do it with stack variables.
Zhao Zhili:
>
>
>> On Apr 11, 2025, at 16:36, Nicolas George <geo...@nsup.org> wrote:
>>
>> Zhao Zhili (HE12025-04-11):
>>> From: Zhao Zhili <zhiliz...@tencent.com>
>>>
>>> The macro is meant to suppress false uninitialized warnings. However,
>>> sometimes these 'false uninitialized warnings' are really undefined
>>> behavior, and leading to real issue like crash, e.g., ab792634197e.
>>>
>>> For false uninitialized warnings, it can be silenced by initialization,
>>> and compiler can easily optimize away unnecessary initializations.
>>>
>>> av_uninit shouldn't be used in any case.
>>
>> NAK, you are hiding the UBs, not fixing the bugs.
>
> By the way, logic bug isn’t equal to UB, so I’m not hiding UB.
>
> Who put av_uninit in the code means there is no logic bug. If there is,
> the patchset fixed UB or replaced UB by deterministic logic error, which
> can’t be worse.
>
>>
>> If the author of the code put av_uninit, that means they believe the
>> value will always have been initialized by the part of the code
>> responsible for it. If that is not true, then it is a bug that can lead
>> to an exploitable security issue or a silent data corruption.
>
> If there is no bug for code with av_uninit, the patchset does nothing really.
> If there is, the patchset fixed or makes the issue deterministic.
4. Wrong: You convey to the reader that this initial value will be used.
And you probably also make the compiler emit code to initialize the value.
>
> We don’t initialized all variables when declaration. But if there is a
> sometimes-uninitialized warning, there is some reason for compiler.
> Uninitialized warning isn’t the same as deprecated or unused, it should
> never be ignored in my opinion.
>
5. The point above is completely wrong. GCC produced tons of these
warnings that were silenced by av_uninit. Until the warning has been
disabled in de6061203e2d509579ab110fb1873aade34320f5. GCC devs in
general seem to not care much about producing bogus warnings (see the
-Wstringop-overflow= warnings produced by aacsbr_template.c). (Clang
warnings about uninitialized values are different though.)
Just take a look at the lavf/electronicarts.c code. You can clearly see
that num_samples is used iff it has been initialized (unless chunk_size
is zero or av_get_packet() errors out). It would only be different if
one of these avio function or av_get_packet() were to change the
demuxer's private context. Using a smaller scope for num_samples (even
when I use a restrict qualified pointer to const to access the private
context in the smaller scope (where the private context is indeed not
changed) to indicate to the compiler that nothing changes the private
context in this scope does not inhibit GCC from emitting
-Wmaybe-uninitialized warning, only -Wno-maybe-uninitialized does. I
really don't know what GCC is thinking here.
But given that we now set -Wno-maybe-uninitialized for GCC, simply
removing these av_uninit does not seem to lead to warnings.
>>
>> With your changes, nothing proves that the = 0 you put there is the
>> right value, the bug is still there: the code expects the value to be
>> correctly set, but instead there is an arbitrary 0.
>>
>> At least, with av_uninit, valgrind and fuzzing can find the bugs.
6. Valgrind is not really the right tool for the job here. Consider code
like the following
int foo;
// something
for (int i = 0; i < count; i++) {
if (condition(i))
foo = bar(i);
}
ctx->baz = foo;
foo may be stored in a register and said register may have already been
written to in "something" and valgrind doesn't know that said register
is now supposed to hold a new variable. msan (memory sanitizer) would be
the proper tool here.
(We btw already have such a Valgrind fate instances:
https://fate.ffmpeg.org/report.cgi?slot=x86_64-archlinux-gcc-valgrind&time=20250411042158)
7. You can't simply make the macro do nothing. It is public API after
all (and a good case for attributes_internal.h, because such a
monstrosity should never have been public in the first place).
Furthermore, at least Clang provides a way to deprecate a macro, so this
should be done. Of course, the needs to be a APIChanges entry, too.
- Andreas
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
