[FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[Zhao Zhili]

From: Zhao Zhili <zhilizhao@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.

Zhao Zhili (22):
  avfilter/af_aecho: Remove use of av_uninit
  avfilter/af_compand: Remove use of av_uninit
  avfilter/vsrc_mandelbrot: Remove use of av_uninit
  avformat/electronicarts: Remove use of av_uninit
  swscale/yuv2rgb: Remove use of av_uninit
  avcodec/ac3enc: Remove use of av_uninit
  avcodec/bfi: Remove use of av_uninit
  avcodec/dvdsubenc: Remove use of av_uninit
  avcodec/eamad: Remove use of av_uninit
  avcodec/ffv1enc: Remove use of av_uninit
  avcodec/flacdec: Remove use of av_uninit
  avcodec/lpc: Remove use of av_uninit
  avcodec/mpeg4videodec: Remove use av_uninit
  avcodec/qtrleenc: Remove use of av_uninit
  avcodec/ra144enc: Remove use av_uninit
  avcodec/vp8: Remove use of av_uninit
  avcodec/wmavoice: Remove use of av_uninit
  avformat/flvdec: Remove use of av_uninit
  avformat/srtp: Remove use of av_uninit
  avformat/wavdec: Remove use of av_uninit
  avformat/tests/seek: Remove use of av_uninit
  avutil/attributes: Make av_uninit do nothing

 libavcodec/ac3enc.c           |  5 +++--
 libavcodec/ac3enc_template.c  | 16 ++++++++--------
 libavcodec/bfi.c              |  2 +-
 libavcodec/dvdsubenc.c        |  2 +-
 libavcodec/eamad.c            |  2 +-
 libavcodec/ffv1enc.c          |  4 ++--
 libavcodec/ffv1enc_template.c |  4 ++--
 libavcodec/flacdec.c          |  2 +-
 libavcodec/lpc.c              |  2 +-
 libavcodec/mpeg4videodec.c    |  2 +-
 libavcodec/qtrleenc.c         |  2 +-
 libavcodec/ra144enc.c         |  4 ++--
 libavcodec/vp8.c              |  2 +-
 libavcodec/wmavoice.c         |  4 ++--
 libavfilter/af_aecho.c        |  2 +-
 libavfilter/af_compand.c      |  2 +-
 libavfilter/vsrc_mandelbrot.c |  2 +-
 libavformat/electronicarts.c  |  2 +-
 libavformat/flvdec.c          |  4 ++--
 libavformat/srtp.c            |  4 ++--
 libavformat/tests/seek.c      |  2 +-
 libavformat/wavdec.c          |  2 +-
 libavutil/attributes.h        |  9 ++++-----
 libswscale/yuv2rgb.c          |  2 +-
 24 files changed, 42 insertions(+), 42 deletions(-)

-- 
2.46.0

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[Nicolas George]

Zhao Zhili (HE12025-04-11):
> From: Zhao Zhili <zhilizhao@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.

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.

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.

Regards,

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[Zhao Zhili]

> On Apr 11, 2025, at 16:36, Nicolas George <george@nsup.org> wrote:
> 
> Zhao Zhili (HE12025-04-11):
>> From: Zhao Zhili <zhilizhao@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.
> 
> 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.
> 
> 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.

With UB, the compiler can remove branch check and assign some random
value to it, which cannot be detected by valgrind.

For ab792634197e, the UB is there for decades and never detected by
valgrind, and the warning is silenced by av_uninit.

> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[Zhao Zhili]

> On Apr 11, 2025, at 16:36, Nicolas George <george@nsup.org> wrote:
> 
> Zhao Zhili (HE12025-04-11):
>> From: Zhao Zhili <zhilizhao@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.

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.

> 
> 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.
> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[Nicolas George]

Zhao Zhili (HE12025-04-11):
> With UB, the compiler can remove branch check and assign some random
> value to it, which cannot be detected by valgrind.
> 
> For ab792634197e, the UB is there for decades and never detected by
> valgrind, and the warning is silenced by av_uninit.

You make a valid point, but my own point still stands: your change make
it harder to find bugs by removing the difference between “I put this
initialization there because it is the right value” and “I put this
there because the compiler is too stupid to figure it on its own”.

I have to ideas to accommodate both issues:

- Have a FATE instance with valgrind and optimizations disabled, so that
  the compiler does not optimize the code away because of the UB and
  valgrind catches the bug.

- Initialize to 0xDEADBEEF and add av_assert2(val != 0xDEADBEEF) at the
  place where it will be used.

The second one might be fragile, but less than an UB.

> By the way, logic bug isn’t equal to UB, so I’m not hiding UB.

Yes you are.

> 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.

The deterministic logic error is not worse, but it is not better either,
and it is harder to detect.

> If there is, the patchset fixed or makes the issue deterministic.

This patches fixes NOTHING, I hope we agree on it. What you did is
basically equivalent to removing an assert that fails and hoping the
code that comes later will be fine.

Making the bug deterministic is not better if it makes it harder to
detect.

> 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.

You are wrong on this, the compiler is often unable to figure out that
the code always sets the variable before reading it when the developer
can prove it. Compilers are getting better, but they are still far from
perfect, and av_unused is precisely there for that lack of perfection,
and needs to stay there.

Regards,

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[Zhao Zhili]

> On Apr 11, 2025, at 17:32, Nicolas George <george@nsup.org> wrote:
> 
> Zhao Zhili (HE12025-04-11):
>> With UB, the compiler can remove branch check and assign some random
>> value to it, which cannot be detected by valgrind.
>> 
>> For ab792634197e, the UB is there for decades and never detected by
>> valgrind, and the warning is silenced by av_uninit.
> 
> You make a valid point, but my own point still stands: your change make
> it harder to find bugs by removing the difference between “I put this
> initialization there because it is the right value” and “I put this
> there because the compiler is too stupid to figure it on its own”.
> 
> I have to ideas to accommodate both issues:
> 
> - Have a FATE instance with valgrind and optimizations disabled, so that
>  the compiler does not optimize the code away because of the UB and
>  valgrind catches the bug.
> 
> - Initialize to 0xDEADBEEF and add av_assert2(val != 0xDEADBEEF) at the
>  place where it will be used.

This is only check on particular compiler implementation. It works until someone
use a different compiler and UB kicks in.

> 
> The second one might be fragile, but less than an UB.
> 
>> By the way, logic bug isn’t equal to UB, so I’m not hiding UB.
> 
> Yes you are.
> 
>> 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.
> 
> The deterministic logic error is not worse, but it is not better either,
> and it is harder to detect.
> 
>> If there is, the patchset fixed or makes the issue deterministic.
> 
> This patches fixes NOTHING, I hope we agree on it. What you did is
> basically equivalent to removing an assert that fails and hoping the
> code that comes later will be fine.
> 
> Making the bug deterministic is not better if it makes it harder to
> detect.
> 
>> 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.
> 
> You are wrong on this, the compiler is often unable to figure out that
> the code always sets the variable before reading it when the developer
> can prove it. Compilers are getting better, but they are still far from
> perfect, and av_unused is precisely there for that lack of perfection,
> and needs to stay there.
> 
> Regards,
> 
> -- 
>  Nicolas George
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[Nicolas George]

Zhao Zhili (HE12025-04-11):
> This is only check on particular compiler implementation. It works until someone
> use a different compiler and UB kicks in.

No, with this suggestion there is no UB.

-- 
  Nicolas George
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".

Re: [FFmpeg-devel] [PATCH 00/22] Deprecate av_uninit

[Andreas Rheinhardt]

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 <george@nsup.org> wrote:
>>
>> Zhao Zhili (HE12025-04-11):
>>> From: Zhao Zhili <zhilizhao@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-request@ffmpeg.org with subject "unsubscribe".