png_write_png (libpng 1.6.21 + zlib 1.2.8) causes uninitiialized read in fill_window

[lilith]

Has this issue been seen before, or should I try to work up a test case to reproduce it?

Conditional jump or move depends on uninitialised value(s) at 0x4F52F22: fill_window by 0x4F536FE: deflate_fast by 0x4F5209F: deflate by 0x4F4B7E1: png_compress_IDAT by 0x4F4EA9C: png_write_filtered_row by 0x4F4EA6D: png_write_find_filter by 0x4F46F41: png_write_row by 0x4F46927: png_write_image by 0x4F478E2: png_write_png Uninitialised value was created by a heap allocation at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) by 0x4F28825: png_malloc_base by 0x4F28A85: png_malloc_warn by 0x4F1FB98: pngzalloc by 0x4F5053A: deflateInit2 by 0x4F4ACB8: png_deflate_claim by 0x4F4B727: png_compress_IDAT by 0x4F4EA9C: png_write_filtered_row by 0x4F4EA6D: png_write_find_filter by 0x4F46F41: png_write_row by 0x4F46927: png_write_image by 0x4F478E2: png_write_png

[glennrp]

These haven't been reported before, so please set up a test case.

[PatrickFerry]

@nathanaeljones poke.

[glennrp]

Closing due to lack of response from OP.

[lilith]

I'm sorry this took so long - it was highly enmeshed with other test code. I've reduced it, and the following appears sufficient to reproduce the bug:

TEST_CASE("Test png writing", "[libpng]"){
    png_structp png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, NULL,
                                                  NULL); // makepng_error, makepng_warning);

    uint32_t w = 300;
    uint32_t h = 200;
    uint32_t stride = 1200;

    uint8_t * pixels = (uint8_t *)calloc(1, stride * h);
    uint8_t ** rows = (uint8_t **)calloc(1, sizeof(uint8_t *) * h);

    unsigned int y;
    for (y = 0; y < h; ++y) {
        rows[y] = ((uint8_t *)pixels + (stride * y));
    }

    png_set_compression_level(png_ptr, Z_BEST_SPEED);
    png_set_text_compression_level(png_ptr, Z_DEFAULT_COMPRESSION);

    png_init_io(png_ptr, fopen("/dev/null","wb"));
    png_infop info_ptr = NULL;
    info_ptr = png_create_info_struct(png_ptr);
    png_set_rows(png_ptr, info_ptr, rows);
    int color_type = PNG_COLOR_TYPE_RGB_ALPHA;
    int transform = PNG_TRANSFORM_BGR;
    png_set_IHDR(png_ptr, info_ptr, (png_uint_32)w, (png_uint_32)h, 8, color_type,
                     PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);
    png_set_sRGB_gAMA_and_cHRM(png_ptr, info_ptr, PNG_sRGB_INTENT_PERCEPTUAL);
    //Uninitialized read happens here:
     png_write_png(png_ptr, info_ptr, transform, NULL);

    png_destroy_write_struct(&png_ptr, &info_ptr);    
    free(pixels);
    free(rows);
}

[lilith]

@glennrp Can we re-open this?

[glennrp]

I replaced the first line with

//TEST_CASE("Test png writing", "[libpng]"){
#include <inttypes.h>
#include <stdio.h>
#include <stdlib.h>
int main()
{
#include <zlib.h>
#include <png.h>
   printf("\n Testing libpng version %s\n", PNG_LIBPNG_VER_STRING);
   printf("   with zlib   version %s\n", ZLIB_VERSION);

Compiled with

gcc -g -fsanitize=address *.c -lpng -lz

Ran with

valgrind a.out

and am not seeing any complaint about UMR.

[lilith]

What version of libpng and zlib are you using? I can try to reproduce it with specific commit IDs that you're using.

[glennrp]

Testing libpng version 1.6.25 with zlib version 1.2.8

Earlier I had the same result with libpng-1.6.22, and also rebuilding with libpng-1.6.21 produces the same.

[lilith]

I can't repro this unless I link it statically. I get other UMRs without building zlib from source myself, but the fill_window occurrence requires me to download and build zlib.

wget -nc https://sourceforge.net/projects/libpng/files/libpng16/older-releases/1.6.23/libpng-1.6.23.tar.gz
wget -nc http://zlib.net/zlib128.zip
tar -xvzf libpng-1.6.23.tar.gz
cd libpng-1.6.23
CFLAGS="-fPIC" ./configure --prefix=$(pwd)/..
make
make install
cd ..
unzip zlib128.zip
cd zlib-1.2.8
CFLAGS="-fPIC" ./configure --prefix=$(pwd)/..
make
make install
cd ..

gcc testread.c -lpng16 -lz -lm -static -L./lib -I./include -o testread.out
valgrind ./testread.out --track-origins=yes

and using this testread.c:

#include <inttypes.h>
#include <stdio.h>
#include <stdlib.h>
#include "zlib.h"
#include "png.h"

int main()
{
    printf("\n Testing libpng version %s\n", PNG_LIBPNG_VER_STRING);
    printf("   with zlib   version %s\n", ZLIB_VERSION);
    png_structp png_ptr = png_create_write_struct(PNG_LIBPNG_VER_STRING, NULL, NULL,
                                                  NULL); // makepng_error, makepng_warning);

    uint32_t w = 300;
    uint32_t h = 200;
    uint32_t stride = 1200;

    uint8_t * pixels = (uint8_t *)calloc(1, stride * h);
    uint8_t ** rows = (uint8_t **)calloc(1, sizeof(uint8_t * ) * h);

    unsigned int y;
    for (y = 0; y < h; ++y) {
        rows[y] = ((uint8_t *)pixels + (stride * y));
    }

    png_set_compression_level(png_ptr, Z_BEST_SPEED);
    png_set_text_compression_level(png_ptr, Z_DEFAULT_COMPRESSION);

    png_init_io(png_ptr, fopen("/dev/null", "wb"));
    png_infop info_ptr = NULL;
    info_ptr = png_create_info_struct(png_ptr);
    png_set_rows(png_ptr, info_ptr, rows);
    int color_type = PNG_COLOR_TYPE_RGB_ALPHA;
    int transform = PNG_TRANSFORM_BGR;
    png_set_IHDR(png_ptr, info_ptr, (png_uint_32)w, (png_uint_32)h, 8, color_type,
                 PNG_INTERLACE_NONE, PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);
    png_set_sRGB_gAMA_and_cHRM(png_ptr, info_ptr, PNG_sRGB_INTENT_PERCEPTUAL);
//Uninitialized read happens here:
    png_write_png(png_ptr, info_ptr, transform, NULL);

    png_destroy_write_struct(&png_ptr, &info_ptr);
    free(pixels);
    free(rows);
    return 0;
}

[lilith]

I'm trying to fetch a release tarball of 1.6.26, but it's been failing for the last 2 hours. I've manually tried every mirror. I can sometimes get a zero byte file.

Other releases are working.

I'm trying to wget https://sourceforge.net/projects/libpng/files/libpng16/1.6.26/libpng-1.6.26.tar.gz

[glennrp]

Sourceforge seems to be out to lunch again. I've just put a copy on github, master branch of glennrp/libpng-releases

Glenn

On Mon, Nov 14, 2016 at 11:48 AM, Nathanael Jones notifications@github.com wrote:

I'm trying to fetch a release tarball of 1.6.26, but it's been failing for the last 2 hours. I've manually tried every mirror. I can sometimes get a zero byte file.

Other releases are working.

I'm trying to wget https://sourceforge.net/projects/libpng/files/ libpng16/1.6.26/libpng-1.6.26.tar.gz

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/glennrp/libpng/issues/97#issuecomment-260390354, or mute the thread https://github.com/notifications/unsubscribe-auth/ABe25hk8Dxrcg1orE7Fkb660LOvBCTagks5q-JDLgaJpZM4H90Ii .

[lilith]

Thanks! I've reproduced this with 1.6.26. Static linking required.

[lilith]

Any ideas why this would only happen when linking libpng statically?

[jbowler]

This is happening on line 1421 of deflate.c, right? Where fill_window copies the "upper half" of the window bufffer allocated by deflateInit2 down to the lower half. I think it's probably happening on row 27.

libpng doesn't know, or control, anything about the internals of zlib and this is all happening inside zlib; the original buffer was allocated in deflateInit2 and the access to the uninitialized parts happens in fill_window.

Doesn't this always happen when deflate is called repeatedly with buffers which are always less than 32768 bytes long and the default allocator is used?

Maybe I'm missing something. It seems to me that if the first buffer copy happens within the first 32768 bytes of the input the window must be uninitialized. I checked 1.2.8 and I checked 1.2.6, I can't see any relevant differences but maybe earlier versions managed to initialize the whole buffer.

[jbowler]

I deleted my previous comment. Visiting this shit on Mark is inappropriate, Mark: accept my apologies if that was the result.

[jbowler]

@ctruta: not a libpng bug and not a bug as the data is not used. Please close.