1663767 - Use of uninitialised value of size 8 in film_grain.asm

Status: RESOLVED WONTFIX

(Core :: Graphics: ImageLib, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.avif

Report created with Valgrind and m-c 20200908-fb9c01b719fa

Thread 16 ImgDecoder #1:
Use of uninitialised value of size 8
   at 0xD6E39DA: ??? (/builds/worker/checkouts/gecko/third_party/dav1d/src/x86/film_grain.asm:1048)
   by 0x45: ???
 Uninitialised value was created by a heap allocation
   at 0x483DE35: memalign (vg_replace_malloc.c:906)
   by 0x483DF37: posix_memalign (vg_replace_malloc.c:1070)
   by 0xD68613F: dav1d_default_picture_alloc (checkouts/gecko/third_party/dav1d/include/common/mem.h:47)
   by 0xD686355: picture_alloc_with_edges (checkouts/gecko/third_party/dav1d/src/picture.c:140)
   by 0xD686246: dav1d_thread_picture_alloc (checkouts/gecko/third_party/dav1d/src/picture.c:186)
   by 0xD66A14E: dav1d_submit_frame (checkouts/gecko/third_party/dav1d/src/decode.c:3423)
   by 0xD681B27: dav1d_parse_obus (checkouts/gecko/third_party/dav1d/src/obu.c:1541)
   by 0xD6801AB: gen_picture (checkouts/gecko/third_party/dav1d/src/lib.c:370)
   by 0xD6800A1: dav1d_send_data (checkouts/gecko/third_party/dav1d/src/lib.c:400)
   by 0xB64FB3F: mozilla::image::nsAVIFDecoder::DecodeWithDav1d(Mp4parseByteData const&, mozilla::layers::PlanarYCbCrData&) (checkouts/gecko/image/decoders/nsAVIFDecoder.cpp:132)
   by 0xB6507BD: mozilla::image::nsAVIFDecoder::DoDecode(mozilla::image::SourceBufferIterator&, mozilla::image::IResumable*) (checkouts/gecko/image/decoders/nsAVIFDecoder.cpp:467)
   by 0xB60A662: mozilla::image::Decoder::Decode(mozilla::image::IResumable*) (checkouts/gecko/image/Decoder.cpp:172)

Comment 1

[Jon Bauman [:jbauman:]]

Filed as a confidential issue with dav1d. Marking as stalled until resolved on the dav1d side or shown to be not dav1d-related.

Comment 2

[Jon Bauman [:jbauman:]]

Per the dav1d issue:

If I'm understanding it correctly it's using an uninitialized pixel value (src) as an index into a lookup table (scaling). This is not an issue since any possible value (0-255) results in a valid memory location.
For a future 10-bit/12-bit implementation this could be an issue since an uninitialized value can be outside the valid pixel range, but for 8-bit it's fine.
I believe valgrind complains about use of uninitialized values that affects control flow or dereferenced memory addresses. It has no way of knowing that it's acceptable in this particular case.
tl;dr: No security implications. Not sure what the best approach would be if we want to fix it anyway though.

I don't have much valgrind knowledge. Is there any way to annotate this to indicate it's ok?

Comment 3

[Jon Bauman [:jbauman:]]

For performance reasons, I think dav1d is unlikely to address this issue. That's understandable since it additionally requires the consumer code to access, or make accessible, regions outside the valid image frame in order to make this exploitable. If we so desired, we could use Dav1dSettings.allocator.alloc_picture_callback to handle the buffer allocation ourselves and ensure the regions outside the frame are initialized, but I don't think it's worthwhile since we're pretty confident that these regions are never written to the surface buffer, and therefore cannot be accessed by code outside Firefox itself.