[FFmpeg-trac] #11344(swresample:new): av_clip functions, use of uninitialised value

[FFmpeg]

#11344: av_clip functions, use of uninitialised value
-------------------------------------+-------------------------------------
             Reporter:               |                    Owner:  (none)
  names_are_hard                     |
                 Type:  defect       |                   Status:  new
             Priority:  normal       |                Component:
                                     |  swresample
              Version:  git-master   |               Resolution:
             Keywords:               |               Blocked By:
             Blocking:               |  Reproduced by developer:  0
Analyzed by developer:  0            |
-------------------------------------+-------------------------------------
Description changed by names_are_hard:

Old description:

> Some av_clip* functions, including at least av_clip_uintp2_c() and
> av_clip_int16_c(), have behaviour dependent on uninitialised values due
> to use in a conditional.
>
> I have guessed the component to be swresample based on file names in
> valgrind trace, my confidence is moderate.
>
> Samples are attached, if Trac will let me.
>
> Repro instructions:
> valgrind --track-origins=yes ffmpeg_g -i some_sample -nostdin -f null -
>
> Tested against master @ 1e76bd2f39.
>
> Truncated example valgrind output:
> {{{
> ==882956== Conditional jump or move depends on uninitialised value(s)
> ==882956==    at 0x118EA3E: av_clip_int16_c (common.h:245)
> ==882956==    by 0x118EA3E: conv_AV_SAMPLE_FMT_FLT_to_AV_SAMPLE_FMT_S16
> (audioconvert.c:79)
> ==882956==    by 0x118EEDB: swri_audio_convert (audioconvert.c:248)
> [...]
> ==882956==  Uninitialised value was created by a heap allocation
> ==882956==    at 0x4848DA0: memalign (in /usr/libexec/valgrind
> /vgpreload_memcheck-amd64-linux.so)
> [...]
> ==882956==    by 0x8CCD08: audio_get_buffer (get_buffer.c:193)
> ==882956==    by 0x8CCD08: avcodec_default_get_buffer2 (get_buffer.c:284)
> }}}
>

> The specific fix should probably find why initialisation is skipped and
> fix the root cause.  This is hard for me to determine, as I have little
> experience with ffmpeg.
>
> A more general fix that I would recommend also employing is to use
> calloc() where possible when allocating structs / object-like entities.
> On "large" OSes, this is likely to have no performance impact, as the OS
> will keep a pool of zero pages available for this purpose.  If there are
> concerns about perf on smaller OSes, a malloc-like function could be
> chosen at build time for these targets.
>
> This would reduce control available to attackers, and make coverage
> guided fuzzing more efficient.  Currently, ffmpeg coverage is non-
> deterministic on many inputs due to this and similar bugs.

New description:

 Some av_clip* functions, including at least av_clip_uintp2_c() and
 av_clip_int16_c(), have behaviour dependent on uninitialised values due to
 use in a conditional.

 I have guessed the component to be swresample based on file names in
 valgrind trace, my confidence is moderate.

 Samples are attached, if Trac will let me.

 Repro instructions:
 valgrind --track-origins=yes ffmpeg_g -i some_sample -nostdin -f null -

 Tested against master @ 1e76bd2f39.

 Truncated example valgrind output:
 {{{
 ==882956== Conditional jump or move depends on uninitialised value(s)
 ==882956==    at 0x118EA3E: av_clip_int16_c (common.h:245)
 ==882956==    by 0x118EA3E: conv_AV_SAMPLE_FMT_FLT_to_AV_SAMPLE_FMT_S16
 (audioconvert.c:79)
 ==882956==    by 0x118EEDB: swri_audio_convert (audioconvert.c:248)
 [...]
 ==882956==  Uninitialised value was created by a heap allocation
 ==882956==    at 0x4848DA0: memalign (in /usr/libexec/valgrind
 /vgpreload_memcheck-amd64-linux.so)
 [...]
 ==882956==    by 0x8CCD08: audio_get_buffer (get_buffer.c:193)
 ==882956==    by 0x8CCD08: avcodec_default_get_buffer2 (get_buffer.c:284)
 }}}


 The specific fix should probably find why initialisation is skipped and
 fix the root cause.  This is hard for me to determine, as I have little
 experience with ffmpeg.  Quite possibly, more av_clip* functions can be
 reached in a similar way, with a different input, given the similarity in
 traces for the two examples provided here.

 A more general fix that I would recommend also employing is to use
 calloc() where possible when allocating structs / object-like entities.
 On "large" OSes, this is likely to have no performance impact, as the OS
 will keep a pool of zero pages available for this purpose.  If there are
 concerns about perf on smaller OSes, a malloc-like function could be
 chosen at build time for these targets.

 This would reduce control available to attackers, and make coverage guided
 fuzzing more efficient.  Currently, ffmpeg coverage is non-deterministic
 on many inputs due to this and similar bugs.

--
-- 
Ticket URL: <https://trac.ffmpeg.org/ticket/11344#comment:1>
FFmpeg <https://ffmpeg.org>
FFmpeg issue tracker