1292402 - Use of uninitialised value in [@mozilla::gfx::FilterProcessing::DoUnpremultiplicationCalculation_SSE2]

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: log.txt

Found with Valgrind.

Use of uninitialised value of size 8
   at 0x11BB8017: _mm_set_epi16 (emmintrin.h:593)
   by 0x11BB8017: _mm_setr_epi16 (emmintrin.h:660)
   by 0x11BB8017: FromU16<__vector(2) long long int> (SIMD.h:837)
   by 0x11BB8017: DoUnpremultiplicationCalculation_SIMD<__vector(2) long long int, __vector(2) long long int> (FilterProcessingSIMD-inl.h:940)
   by 0x11BB8017: mozilla::gfx::FilterProcessing::DoUnpremultiplicationCalculation_SSE2(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, unsigned char*, int, unsigned char*, int) (FilterProcessingSSE2.cpp:95)

Uninitialised value was created by a heap allocation
   at 0x4C2DB8F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
   by 0x11BF0C70: Realloc (Tools.h:181)
   by 0x11BF0C70: mozilla::gfx::SourceSurfaceAlignedRawData::Init(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::SurfaceFormat, bool) (SourceSurfaceRawData.cpp:61)

see log.txt for full log.

Comment 1

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Comment 2

[Lee Salzman [:lsalzman]]

In DoUnpremultiplicationCalculation_SSE2 and friends, should we be verifying the size is actually a multiple of 4? We don't seem to be doing so, but in the test case the size requested for the canvas is definitely not.

Comment 3

[Markus Stange [:mstange]]

This code is somewhat intentionally using uninitialized values and may even propagate values derived from them into other surfaces, but the idea is that all those values will be ignored in the final surface (and during the final draw operation) because they'll be outside the declared size of the surface.

That said, it probably wouldn't hurt too much to zero out just the alignment padding at the right edge of the surface after allocation.

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1292402.patch

I can't reproduce the warning, but this patch should fix it.

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Comment on attachment 8780169 [details] [diff] [review]
1292402.patch

On second thought, this happens a bunch of times in that file. I'll go through and put up a more exhaustive patch.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8780169 [details] [diff] [review]
1292402.patch

Review of attachment 8780169 [details] [diff] [review]:
-----------------------------------------------------------------

thanks

::: gfx/2d/FilterNodeSoftware.cpp
@@ +1724,5 @@
>        }
>      }
> +
> +    // Zero padding to keep valgrind happy.
> +    memset(&targetData[y * targetStride + sourceStride * BytesPerPixel], 0,

Could you use PodZero instead?

Comment 7

[Markus Stange [:mstange]]

oh

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1292402.patch

This should be all of it.

There was a bug in the original patch as well, which assumed that the stride was in pixels instead of bytes. Then valgrind would have had two things to complain about. :)

Comment 9

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b0e580940a42734ceada1375199f04bf0e138080

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b0e580940a42