| Summary: | 'test/stress-test' segfaults on i686 (pixman 0.32.6) | ||
|---|---|---|---|
| Product: | pixman | Reporter: | Ludovic Courtès <ludo> |
| Component: | pixman | Assignee: | Søren Sandmann Pedersen <soren.sandmann> |
| Status: | RESOLVED FIXED | QA Contact: | Søren Sandmann Pedersen <soren.sandmann> |
| Severity: | major | ||
| Priority: | medium | CC: | ludo, oded.gabbay, siarhei.siamashka |
| Version: | other | ||
| Hardware: | x86 (IA32) | ||
| OS: | Linux (All) | ||
| Whiteboard: | |||
| i915 platform: | i915 features: | ||
| Attachments: |
Fix use of uninitialized value
Avoid numerical overflow in pointer arithmetic. |
||
|
Description
Ludovic Courtès
2015-09-16 19:32:48 UTC
I can provide instructions on how to reproduce it using GNU Guix. Created attachment 118319 [details] [review] Fix use of uninitialized value The attached patch placates Valgrind but I'm still getting segfaults in the build container that I cannot reproduce outside of the container. This also concerns the development snapshot 0.33.2. I can't reproduce the problem on my 64-bit system (using CFLAGS="-O2 -m32" to build 32-bit code).(In reply to Ludovic Courtès from comment #2) > Created attachment 118319 [details] [review] [review] > Fix use of uninitialized value > > The attached patch placates Valgrind but I'm still getting segfaults in the > build container that I cannot reproduce outside of the container. The uninitialized value alarm is a known quirk, explained here with more details: http://lists.freedesktop.org/archives/pixman/2013-December/003164.html Using memset is not desired in the performance critical part of code, so we would want a better solution. Relying on TLS (as mentioned in the mailing list) is not a very good option either because we also have to support weird operating systems, such as Microsoft Windows. See bug #34842 In any case, it is very unlikely to be related to your segfault. (In reply to Ludovic Courtès from comment #1) > I can provide instructions on how to reproduce it using GNU Guix. I tried to install GCC 4.9.3, but still can't reproduce the problem. Additional information would be appreciated. Also please try "--disable-openmp" configure option (so that we don't need "OMP_NUM_THREADS=1") and also "--disable-gtk --disable-libpng --disable-shared --enable-static-testprogs" as an additional experiment. Removing OpenMP and libtool out of the way may make debugging easier. (The trick to deterministically reproduce the issue was to turn off ASLR either in GDB or with 'setarch -R', which is what Guix's build daemon does.)
After some more debugging, I found what seems to be the root cause. 'general_composite_rect' allocates a fixed-size buffer on the stack and then computes 'src_buffer' and 'dest_buffer' as a function of that buffer's address and 'width'.
The problem is that the buffer address is typically high (since it's on the stack), so for high values of 'width', adding 'width * Bpp' to that address can overflow. Thus, the test to determine whether to fall back to heap allocation:
if (ALIGN (dest_buffer + width * Bpp) >
scanline_buffer + sizeof (stack_scanline_buffer))
will just fail, because 'dest_buffer + width * Bpp' is numerically smaller than 'scanline_buffer'. Here's an example GDB session:
(gdb) p dest_buffer
$1 = (uint8_t *) 0xd0f0 <error: Cannot access memory at address 0xd0f0>
(gdb) p src_buffer
$2 = (uint8_t *) 0xffff6150 ""
(gdb) p mask_buffer
$3 = (uint8_t *) 0x1920 <error: Cannot access memory at address 0x1920>
(gdb) p src_buffer
$4 = (uint8_t *) 0xffff6150 ""
(gdb) p width
$5 = 11764
(gdb) p src_buffer + width
$6 = (uint8_t *) 0xffff8f44 ""
(gdb) p Bpp
$7 = <optimized out>
(gdb) p src_buffer + width*16
$8 = (uint8_t *) 0x24090 <error: Cannot access memory at address 0x24090>
(gdb) p mask_buffer
$9 = (uint8_t *) 0x1920 <error: Cannot access memory at address 0x1920>
(gdb) p src_buffer + width*8
$10 = (uint8_t *) 0xd0f0 <error: Cannot access memory at address 0xd0f0>
(gdb) p src_buffer + width*4
$11 = (uint8_t *) 0x1920 <error: Cannot access memory at address 0x1920>
(gdb) p scanline_buffer
$12 = (uint8_t *) 0xffff6150 ""
(gdb) p src_buffer
$13 = (uint8_t *) 0xffff6150 ""
(gdb) p width
$14 = 11764
I'm attaching a patch which I think solves the issue correctly. Please let me know what you think.
Created attachment 118382 [details] [review] Avoid numerical overflow in pointer arithmetic. Thanks for debugging this and also for the detailed analysis of the problem! This looks like a rather important bugfix. Please send your patch to the pixman mailing list for review: http://lists.freedesktop.org/mailman/listinfo/pixman Though based on a quick look, the first revision of the patch does not seem to be entirely correct because the ALIGN macro is not fully accounted for. This macro ensures 16 bytes alignment, which is needed for SIMD optimized code. And using just (width + 1) is not sufficient for this. Also this whole part of code may need to rewritten a bit, because calculating addresses beyond the array bounds even without dereferencing them is a bad idea: http://stackoverflow.com/questions/988158/take-the-address-of-a-one-past-the-end-array-element-via-subscript-legal-by-the Patch that fixes this problem was pushed to master and to 0.32 branch. A new stable release, 0.32.8, will be made very soon to include this patch |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.