Re: [FFmpeg-devel] [PATCH v2 7/7] avcodec: add AV_CODEC_FLAG_CLEAR

["Rémi Denis-Courmont" <remi@remlab.net>]

Hi,

Le 8 décembre 2023 00:47:13 GMT+02:00, Marton Balint <cus@passwd.hu> a écrit :
>
>
>On Thu, 7 Dec 2023, Anton Khirnov wrote:
>
>> Quoting Ronald S. Bultje (2023-12-07 02:44:36)
>>> Hi,
>>> 
>>> On Wed, Dec 6, 2023 at 3:23 AM Marton Balint <cus@passwd.hu> wrote:
>>> 
>>> > Signed-off-by: Marton Balint <cus@passwd.hu>
>>> > ---
>>> >  doc/APIchanges             |  3 +++
>>> >  doc/codecs.texi            | 14 ++++++++++++++
>>> >  libavcodec/avcodec.h       |  4 ++++
>>> >  libavcodec/decode.c        |  6 ++++++
>>> >  libavcodec/options_table.h |  1 +
>>> >  libavcodec/version.h       |  2 +-
>>> >  6 files changed, 29 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/doc/APIchanges b/doc/APIchanges
>>> > index 416e2bec5e..f839504a64 100644
>>> > --- a/doc/APIchanges
>>> > +++ b/doc/APIchanges
>>> > @@ -2,6 +2,9 @@ The last version increases of all libraries were on
>>> > 2023-02-09
>>> >
>>> >  API changes, most recent first:
>>> >
>>> > +2023-12-xx - xxxxxxxxxxx - lavc 60.36.100 - avcodec.h
>>> > +  Add AV_CODEC_FLAG_CLEAR.
>>> > +
>>> >  2023-12-xx - xxxxxxxxxxx - lavu 58.33.100 - imgutils.h
>>> >    Add av_image_fill_color()
>>> >
>>> > diff --git a/doc/codecs.texi b/doc/codecs.texi
>>> > index 5b950b4560..0504a535f2 100644
>>> > --- a/doc/codecs.texi
>>> > +++ b/doc/codecs.texi
>>> > @@ -76,6 +76,20 @@ Apply interlaced motion estimation.
>>> >  Use closed gop.
>>> >  @item output_corrupt
>>> >  Output even potentially corrupted frames.
>>> > +@item clear
>>> > +Clear the contents of the video buffer before decoding the next picture
>>> > to it.
>>> > +
>>> > +Usually if only a part of a picture is affected by a decode error then the
>>> > +decoder (if it implements error concealment) tries to hide it by
>>> > interpolating
>>> > +pixels from neighbouring areas or in some cases from the previous frame.
>>> > Even
>>> > +without error concealment it is quite likely that the affected area will
>>> > +contain pixels from an earlier frame, due to frame pooling.
>>> >
>>> 
>>> No comment on the patch itself, but wouldn't our users (and the C standard
>>> itself) consider it a security issue to return stale
>> 
>> I don't see the security issue in returning previously-returned frame
>> data.
>
>I guess what Ronald means that it is possible that the decoder frame pool allocates data in heap previously containing sensitive data, and that might never get overwritten in case of faulty input before passing it to the user.
>
>The simple fix for that is to clear frame pool buffers on creation?
>
>I am not sure if it is actually UB to read uninitialzied data from the heap though.

Reading uninitialised data is UB if the type representation is not surjective (e.g. bool, and potentially compound types with padding). Of course there are all sorts of other problems that could indirectly cause UB such as implicitly assuming that an integer fits a certain range and triggering an undefined overflow otherwise.

>
>Regards,
>Marton
>_______________________________________________
>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".