Bug 697151 - png: Accessing uninitialized data when unpacking
Summary: png: Accessing uninitialized data when unpacking
Status: RESOLVED FIXED
Alias: None
Product: MuPDF
Classification: Unclassified
Component: fuzzing (show other bugs)
Version: master
Hardware: All All
: P4 normal
Assignee: MuPDF bugs
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-26 04:22 UTC by Sebastian Rasmussen
Modified: 2016-10-12 07:37 UTC (History)
1 user (show)

See Also:
Customer:
Word Size: ---

Attachments
Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Rasmussen 2016-09-26 04:22:38 UTC 
When running this png image:

https://raw.githubusercontent.com/Grumbel/imagetestsuite/master/png/e59ec0cfb8ab64558099543dc19f8378.png

using the following command:

valgrind --track-origins=yes ./build/debug/mutool draw -i -s t e59ec0cfb8ab64558099543dc19f8378.png

I see warnings about accessing uninitialized data:

==11632== Using Valgrind-3.12.0.SVN and LibVEX
[...]
==11524== Conditional jump or move depends on uninitialised value(s)
==11524==    at 0x4031D29: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x810728A: fz_unpack_tile (draw-unpack.c:121)
==11524==    by 0x810A6D6: fz_load_png (load-png.c:586)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524==    by 0x804ABE6: drawband (mudraw.c:554)
==11524==    by 0x804BFE8: dodrawpage (mudraw.c:914)
==11524==    by 0x804CD20: drawpage (mudraw.c:1207)
==11524==    by 0x804CD8F: drawrange (mudraw.c:1224)
==11524==  Uninitialised value was created by a heap allocation
==11524==    at 0x402D27C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x809BCB5: fz_malloc_default (memory.c:213)
==11524==    by 0x809B787: do_scavenging_malloc (memory.c:17)
==11524==    by 0x809B916: fz_malloc_array (memory.c:80)
==11524==    by 0x81095C4: png_deinterlace (load-png.c:190)
==11524==    by 0x810A2BA: png_read_image (load-png.c:490)
==11524==    by 0x810A5A8: fz_load_png (load-png.c:564)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524== 
==11524== Conditional jump or move depends on uninitialised value(s)
==11524==    at 0x4031D2F: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x810728A: fz_unpack_tile (draw-unpack.c:121)
==11524==    by 0x810A6D6: fz_load_png (load-png.c:586)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524==    by 0x804ABE6: drawband (mudraw.c:554)
==11524==    by 0x804BFE8: dodrawpage (mudraw.c:914)
==11524==    by 0x804CD20: drawpage (mudraw.c:1207)
==11524==    by 0x804CD8F: drawrange (mudraw.c:1224)
==11524==  Uninitialised value was created by a heap allocation
==11524==    at 0x402D27C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x809BCB5: fz_malloc_default (memory.c:213)
==11524==    by 0x809B787: do_scavenging_malloc (memory.c:17)
==11524==    by 0x809B916: fz_malloc_array (memory.c:80)
==11524==    by 0x81095C4: png_deinterlace (load-png.c:190)
==11524==    by 0x810A2BA: png_read_image (load-png.c:490)
==11524==    by 0x810A5A8: fz_load_png (load-png.c:564)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524== 
==11524== Conditional jump or move depends on uninitialised value(s)
==11524==    at 0x4031D3E: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x810728A: fz_unpack_tile (draw-unpack.c:121)
==11524==    by 0x810A6D6: fz_load_png (load-png.c:586)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524==    by 0x804ABE6: drawband (mudraw.c:554)
==11524==    by 0x804BFE8: dodrawpage (mudraw.c:914)
==11524==    by 0x804CD20: drawpage (mudraw.c:1207)
==11524==    by 0x804CD8F: drawrange (mudraw.c:1224)
==11524==  Uninitialised value was created by a heap allocation
==11524==    at 0x402D27C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x809BCB5: fz_malloc_default (memory.c:213)
==11524==    by 0x809B787: do_scavenging_malloc (memory.c:17)
==11524==    by 0x809B916: fz_malloc_array (memory.c:80)
==11524==    by 0x81095C4: png_deinterlace (load-png.c:190)
==11524==    by 0x810A2BA: png_read_image (load-png.c:490)
==11524==    by 0x810A5A8: fz_load_png (load-png.c:564)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524== 
==11524== Conditional jump or move depends on uninitialised value(s)
==11524==    at 0x4031D85: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x810728A: fz_unpack_tile (draw-unpack.c:121)
==11524==    by 0x810A6D6: fz_load_png (load-png.c:586)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524==    by 0x804ABE6: drawband (mudraw.c:554)
==11524==    by 0x804BFE8: dodrawpage (mudraw.c:914)
==11524==    by 0x804CD20: drawpage (mudraw.c:1207)
==11524==    by 0x804CD8F: drawrange (mudraw.c:1224)
==11524==  Uninitialised value was created by a heap allocation
==11524==    at 0x402D27C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x809BCB5: fz_malloc_default (memory.c:213)
==11524==    by 0x809B787: do_scavenging_malloc (memory.c:17)
==11524==    by 0x809B916: fz_malloc_array (memory.c:80)
==11524==    by 0x81095C4: png_deinterlace (load-png.c:190)
==11524==    by 0x810A2BA: png_read_image (load-png.c:490)
==11524==    by 0x810A5A8: fz_load_png (load-png.c:564)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524== 
==11524== Use of uninitialised value of size 4
==11524==    at 0x4031F10: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x810728A: fz_unpack_tile (draw-unpack.c:121)
==11524==    by 0x810A6D6: fz_load_png (load-png.c:586)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524==    by 0x804ABE6: drawband (mudraw.c:554)
==11524==    by 0x804BFE8: dodrawpage (mudraw.c:914)
==11524==    by 0x804CD20: drawpage (mudraw.c:1207)
==11524==    by 0x804CD8F: drawrange (mudraw.c:1224)
==11524==  Uninitialised value was created by a heap allocation
==11524==    at 0x402D27C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x809BCB5: fz_malloc_default (memory.c:213)
==11524==    by 0x809B787: do_scavenging_malloc (memory.c:17)
==11524==    by 0x809B916: fz_malloc_array (memory.c:80)
==11524==    by 0x81095C4: png_deinterlace (load-png.c:190)
==11524==    by 0x810A2BA: png_read_image (load-png.c:490)
==11524==    by 0x810A5A8: fz_load_png (load-png.c:564)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524== 
==11524== Use of uninitialised value of size 4
==11524==    at 0x4031F12: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x810728A: fz_unpack_tile (draw-unpack.c:121)
==11524==    by 0x810A6D6: fz_load_png (load-png.c:586)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524==    by 0x804ABE6: drawband (mudraw.c:554)
==11524==    by 0x804BFE8: dodrawpage (mudraw.c:914)
==11524==    by 0x804CD20: drawpage (mudraw.c:1207)
==11524==    by 0x804CD8F: drawrange (mudraw.c:1224)
==11524==  Uninitialised value was created by a heap allocation
==11524==    at 0x402D27C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x809BCB5: fz_malloc_default (memory.c:213)
==11524==    by 0x809B787: do_scavenging_malloc (memory.c:17)
==11524==    by 0x809B916: fz_malloc_array (memory.c:80)
==11524==    by 0x81095C4: png_deinterlace (load-png.c:190)
==11524==    by 0x810A2BA: png_read_image (load-png.c:490)
==11524==    by 0x810A5A8: fz_load_png (load-png.c:564)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524== 
==11524== Conditional jump or move depends on uninitialised value(s)
==11524==    at 0x4031F1A: memcpy (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x810728A: fz_unpack_tile (draw-unpack.c:121)
==11524==    by 0x810A6D6: fz_load_png (load-png.c:586)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
==11524==    by 0x804ABE6: drawband (mudraw.c:554)
==11524==    by 0x804BFE8: dodrawpage (mudraw.c:914)
==11524==    by 0x804CD20: drawpage (mudraw.c:1207)
==11524==    by 0x804CD8F: drawrange (mudraw.c:1224)
==11524==  Uninitialised value was created by a heap allocation
==11524==    at 0x402D27C: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==11524==    by 0x809BCB5: fz_malloc_default (memory.c:213)
==11524==    by 0x809B787: do_scavenging_malloc (memory.c:17)
==11524==    by 0x809B916: fz_malloc_array (memory.c:80)
==11524==    by 0x81095C4: png_deinterlace (load-png.c:190)
==11524==    by 0x810A2BA: png_read_image (load-png.c:490)
==11524==    by 0x810A5A8: fz_load_png (load-png.c:564)
==11524==    by 0x808B87E: compressed_image_get_pixmap (image.c:420)
==11524==    by 0x808C288: fz_get_pixmap_from_image (image.c:674)
==11524==    by 0x809840B: fz_draw_fill_image (draw-device.c:1425)
==11524==    by 0x80760C9: fz_fill_image (device.c:319)
==11524==    by 0x8084812: fz_run_display_list (list-device.c:1646)
Comment 1 Sebastian Rasmussen 2016-09-26 05:06:25 UTC 
This should probably be marked as fuzzing, sorry for the noise.
Comment 2 Sebastian Rasmussen 2016-09-28 04:26:53 UTC 
So the png in question is 18 x 11 pixels, 1 component, 1 bits per pixel.

png_deinterlace() allocates a new buffer of size 11 * 3 bytes as 3 bytes for each line is enough to contain the 18 bits per scanline. the padding of 6 bits per scanline is left uninitialized.

Later on fz_unpack_tile() appears to memcpy() this uninitialized padding. Now yet sure why.
Comment 3 Robin Watts 2016-10-12 07:21:51 UTC 
This is a false positive from valgrind.

Worked around in:

commit c5d25481e4409e0f8d3e60781f908ac91c342dfc
Author: Sebastian Rasmussen <sebras@gmail.com>
Date:   Wed Sep 28 19:35:45 2016 +0800

    Bug 697151: Pacify valgrind when reading padding while unpacking tiles

    If e.g. an 18 x 11 PNG image used 1 bit depth and a single component
    the source data fed to fz_unpack_tile() would have padding bits at the
    end of each line of whole bytes, because 18 bits is not a multiple of 8.

    The optimized 1 component versions of fz_unpack_tile() for a bit depth
    of 1 deliberately read the padding when doing the table lookups, knowing
    that this does not matter because of the construction of the table.
    Valgrind is incapable of understanding this, so we provide some extra
    masking (used only in PACIFY_VALGRIND builds) that explicitly mask
    off the invalid bits.
Comment 4 Robin Watts 2016-10-12 07:37:27 UTC 
oops, wrong SHA. Correct commit is:

commit 59f74777cabca2c960411f0d3e0ae8ffd2d4aaed
Author: Sebastian Rasmussen <sebras@gmail.com>
Date:   Wed Sep 28 19:35:45 2016 +0800

    Bug 697151: Pacify valgrind when reading padding while unpacking tiles

    If e.g. an 18 x 11 PNG image used 1 bit depth and a single component
    the source data fed to fz_unpack_tile() would have padding bits at the
    end of each line of whole bytes, because 18 bits is not a multiple of 8.

    The optimized 1 component versions of fz_unpack_tile() for a bit depth
    of 1 deliberately read the padding when doing the table lookups, knowing
    that this does not matter because of the construction of the table.
    Valgrind is incapable of understanding this, so we provide some extra
    masking (used only in PACIFY_VALGRIND builds) that explicitly mask
    off the invalid bits.