[Pixman] Use of uninitialized values (?)

[Siarhei Siamashka]

On Thu, 19 Dec 2013 22:01:46 +0100
Christian Stadelmann <chris.privat at genodeftest.de> wrote:

> Hi
> 
> I just analyzed some application using valgrind. I am getting warnings
> like this:
> 
> ==6514== Conditional jump or move depends on uninitialised value(s)
> ==6514==    at 0x3C8DA72B04: sse2_combine_over_u (pixman-sse2.c:646)
> ==6514==    by 0x3C8DA57CE3: general_composite_rect
> (pixman-general.c:199)
> ==6514==    by 0x3C8DA0B8B0: pixman_image_composite32 (pixman.c:707)
> ==6514==    by 0x3998035976: _inplace_spans
> (cairo-image-compositor.c:2477)
> ==6514==    by 0x3998078F33: _cairo_tor_scan_converter_generate
> (cairo-tor-scan-converter.c:1600)
> ==6514==    by 0x399806B46B: composite_polygon.isra.9
> (cairo-spans-compositor.c:801)
> ==6514==    by 0x399806BEDA: clip_and_composite_polygon
> (cairo-spans-compositor.c:967)
> ==6514==    by 0x399806CEA0: _cairo_spans_compositor_fill
> (cairo-spans-compositor.c:1174)
> ==6514==    by 0x39980290BE: _cairo_compositor_fill
> (cairo-compositor.c:203)
> ==6514==    by 0x3998039C5E: _cairo_image_surface_fill
> (cairo-image-surface.c:985)
> ==6514==    by 0x399806FF43: _cairo_surface_fill (cairo-surface.c:2305)
> ==6514==    by 0x399803128B: _cairo_gstate_fill (cairo-gstate.c:1317)
> ==6514==  Uninitialised value was created by a stack allocation
> ==6514==    at 0x3C8DA57A60: general_composite_rect
> (pixman-general.c:111)
> 
> They have one thing in common: the uninitialised value is created at the
> same position.
> I am not quite solid in writing C code but isn't the value 'op'
> uninitialized in this function?

Hi,

Thanks for this information. I'm pretty sure similar problems have been
reported and discussed earlier, but I can't find a reference offhand.

In your log, valgrind complains about the following loop
in "core_combine_over_u_sse2_mask" function:

    while (w)
    {
        d = *pd;
        s = combine1 (ps, pm);

        if (s)
            *pd = core_combine_over_u_pixel_sse2 (s, d);
        pd++;
        ps++;
        pm++;

        w--;
    }

Can you try to rearrange the code in the following way and test again?

    while (w)
    {
        s = combine1 (ps, pm);

        if (ALPHA_8 (s) == 0xFF)
        {
            *pd = s;
        }
        else if (s)
        {
            d = *pd;
            *pd = core_combine_over_u_pixel_sse2 (s, d);
        }
        pd++;
        ps++;
        pm++;

        w--;
    }

It is likely that 's' is always zero here (for the cases when valgrind
complains), so the end result does not depend on the uninitialized
value. Or it might be that 's' is opaque (alpha = 0xFF), so that the
'd' value does not affect the end result either. But it is better not
to read it in the first place.

The general pipeline works by decomposing the operation into
separate "fetch -> combine -> writeback" steps roughly in the
following way:
1. Fetch source scanline into a temporary buffer
2. Fetch mask scanline into a temporary buffer
3. Fetch destination scanline into a temporary buffer
4. Perform combine operation on temporary buffers
5. Write back to the destination from the temporary buffer

The temporary buffers are allocated on stack (or from the heap if
they are too large), but not initialized at the time of allocation
for performance reasons. Then pixman may see that fetching data into
one of these buffers does not affect the end result and skip the fetch
stage for it altogether. But the combine operation still may read the
data from this uninitialized temporary buffer.

As a workaround (instead of plugging such holes all over the place),
we might allocate the temporary buffers for general path, initialize
them once and keep the pointers to these buffers in TLS. However this
is not very nice and may hide real problems.

By the way, going through the general path is not very good for
performance. It would be really interesting to see why it does not
hit some sse2 optimized fast path in this particular case.

-- 
Best regards,
Siarhei Siamashka