[Pixman] [PATCH] Trying to reproduce bug #97938
Siarhei Siamashka
siarhei.siamashka at gmail.com
Thu Oct 6 04:13:01 UTC 2016
On Wed, 5 Oct 2016 20:25:51 +0200
Alessandro Vesely <vesely at tana.it> wrote:
> See https://bugs.freedesktop.org/show_bug.cgi?id=97938
Hello Alessandro,
Thanks for moving the discussion to the pixman mailing list
and also for trying to construct a testcase for this particular
problem.
By the way, I forgot to ask it in bugzilla. How was this
problem discovered in the first place? Did you use some
static analysis tool? Or maybe had an application crash?
As for extending the test suite, we already have 'stress-test'.
It tries negative strides among other things and also uses
fence_malloc (a special implementation, which aligns the buffer
to a mapped/unmapped page boundary). Looks like we probably want
to add a special code for testing huge images there too.
This bug is likely triggered when dealing with 2GB or larger images.
This is rather inconvenient because sometimes there is not much free
RAM available. Probably instead we want to test a small image, but
with huge strides. And allocate the buffer in such a way that the
gaps between scanlines are not backed by the real physical memory
pages.
Now the main remaining question is whether the stress-test is
providing sufficient coverage and whether the problematic code
parts are getting visited. To do some experiments with it, I used
the following patch:
diff --git a/pixman/pixman-fast-path.c b/pixman/pixman-fast-path.c
index b4daa26..11d4b6f 100644
--- a/pixman/pixman-fast-path.c
+++ b/pixman/pixman-fast-path.c
@@ -2913,8 +2913,11 @@ bits_image_fetch_bilinear_affine (pixman_image_t * image,
repeat (repeat_mode, &x2, width);
repeat (repeat_mode, &y2, height);
- row1 = (uint8_t *)bits->bits + bits->rowstride * 4 * y1;
- row2 = (uint8_t *)bits->bits + bits->rowstride * 4 * y2;
+ /* Simulate integer overflow (truncate intermediate result to NBITS bits) */
+ #define NBITS 15
+ #define XBITS (32 - NBITS)
+ row1 = (uint8_t *)bits->bits + ((int32_t)((bits->rowstride * 4 * y1) << XBITS) >> XBITS);
+ row2 = (uint8_t *)bits->bits + ((int32_t)((bits->rowstride * 4 * y2) << XBITS) >> XBITS);
tl = convert_pixel (row1, x1) | mask;
tr = convert_pixel (row1, x2) | mask;
The idea is that testing 32-bit overflows is a PITA because we need
really huge images. But the code can be changed to simulate overflows
with any arbitrary number of bits (which are defined by the NBITS
constant). If I set NBITS to 16 or more, then the pixman tests
still pass. If I reduce NBITS to 15, then I get exactly the
'stress-test' crashing:
PASS: trap-crasher
PASS: oob-test
PASS: infinite-loop
PASS: region-translate-test
PASS: fetch-test
PASS: a1-trap-test
PASS: prng-test
PASS: radial-invalid
PASS: pdf-op-test
PASS: fence-image-self-test
PASS: region-test
PASS: combiner-test
PASS: alpha-loop
PASS: scaling-crash-test
PASS: scaling-helpers-test
PASS: thread-test
PASS: rotate-test
PASS: gradient-crash-test
PASS: alphamap
PASS: matrix-test
PASS: pixel-test
PASS: composite-traps-test
PASS: region-contains-test
PASS: filter-reduction-test
PASS: glyph-test
PASS: solid-test
PASS: blitters-test
PASS: cover-test
PASS: affine-test
PASS: scaling-test
PASS: composite
PASS: tolerance-test
../test-driver: line 107: 3527 Segmentation fault "$@" > $log_file 2>&1
FAIL: stress-test
If I further reduce NBITS to 11, then I get some other tests failing:
PASS: oob-test
PASS: trap-crasher
PASS: infinite-loop
PASS: region-translate-test
PASS: fetch-test
PASS: a1-trap-test
PASS: prng-test
PASS: radial-invalid
PASS: combiner-test
PASS: region-test
PASS: fence-image-self-test
PASS: alpha-loop
PASS: scaling-crash-test
PASS: scaling-helpers-test
PASS: rotate-test
PASS: thread-test
PASS: pdf-op-test
PASS: gradient-crash-test
PASS: alphamap
PASS: pixel-test
PASS: matrix-test
PASS: filter-reduction-test
PASS: composite-traps-test
PASS: region-contains-test
PASS: glyph-test
PASS: solid-test
PASS: blitters-test
PASS: cover-test
FAIL: affine-test
FAIL: scaling-test
PASS: composite
PASS: tolerance-test
PASS: stress-test
A tricky part here is that the stress-test crash disappeared too. This
can be explained by the fact that the offsets become smaller and wrong
memory accesses still hit some mapped memory inside of heap.
Anyway, the affected problematic code from 'pixman-fast-path.c' is
already covered by the stress-test and this is good.
> I'm not sure the image I pass is valid; if not, that would be a Cairo
> bug or something in between. Tomorrow I'll send more patches.
>
> Ciao
> Ale
>
> Signed-off-by: Alessandro Vesely <vesely at tana.it>
> ---
> test/large-test.c | 77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 77 insertions(+)
> create mode 100644 test/large-test.c
>
> diff --git a/test/large-test.c b/test/large-test.c
> new file mode 100644
> index 0000000..bf9095a
> --- /dev/null
> +++ b/test/large-test.c
> @@ -0,0 +1,77 @@
> +#include <stdlib.h>
> +#include "utils.h"
> +
> +#if 0
> +static void
> +on_destroy (pixman_image_t *image, void *data)
> +{
> + free (data);
> +}
> +
> +pixman_image_set_destroy_function (image, on_destroy, orig);
> +#endif
> +
> +static pixman_image_t *
> +make_image (pixman_format_code_t format, size_t width, size_t height)
> +{
> + uint32_t *bytes, *orig;
> + pixman_image_t *image;
> + int stride;
> +
> + size_t size = width * height * 4UL;
> + orig = bytes = calloc (1, size);
> + if (!orig)
> + {
> + puts("failed");
> + return NULL;
> + }
> + prng_randmemset (bytes, size, 0);
> +
> + stride = width * 4;
> + image = pixman_image_create_bits (
> + format, width, height, bytes, stride);
> +
> + pixman_image_set_repeat (image, PIXMAN_REPEAT_NONE);
> +
> + image_endian_swap (image);
> + return image;
> +}
> +
> +static uint32_t
> +run_test(int testnum, int verbose)
> +{
> + static const pixman_transform_t transform =
> + {{{2476867, 0, 84}, {0, 2476867, 44980853}, {0, 0, 65536}}};
> +
> + pixman_image_t *src, *mask, *dest;
> +
> + prng_srand(testnum);
> +
> + /*
> + * This is meant to be similar to a call from
> + * cairo-1.14.0/src/cairo-image-compositor.c:2496
> + */
> + src = make_image(PIXMAN_a8r8g8b8, 19866, 28087);
> + pixman_image_set_transform (src, &transform);
> + mask = make_image(PIXMAN_a8, 528, 32);
> + dest = make_image(PIXMAN_a8r8g8b8, 526, 32);
> + pixman_image_composite32(PIXMAN_OP_SRC,
> + src,
> + mask,
> + dest,
> + 0 /* src_x */,
> + 693 /* src_y */,
> + 0 /* mask_x */,
> + 0 /* mask_y */,
> + 0 /* dest_x */,
> + 0 /* dest_y */,
> + 525 /* width */,
> + 32 /* height */);
> + return 1;
> +}
> +
> +int
> +main (int argc, const char *argv[])
> +{
> + return fuzzer_test_main ("large", 1, 1, run_test, argc, argv);
> +}
We surely can use a separate test for it. But extending the
existing 'stress-test' is probably a better idea in general.
Once we have the test suite adjusted, we are expected to find
most of the bugs of this kind and also safeguard against them
in the future.
What do you think?
--
Best regards,
Siarhei Siamashka
More information about the Pixman
mailing list