On Wed, 5 Oct 2016 20:25:51 +0200 Alessandro Vesely <ves...@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 <ves...@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 _______________________________________________ Pixman mailing list Pixman@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/pixman