Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SEGV in function pngwriter::readfromfile #129

Closed
fouzhe opened this issue Jul 11, 2018 · 16 comments
Closed

SEGV in function pngwriter::readfromfile #129

fouzhe opened this issue Jul 11, 2018 · 16 comments

Comments

@fouzhe
Copy link

fouzhe commented Jul 11, 2018

I use Clang 6.0 and AddressSanitizer to build pngwriter v0.7.0, this file can cause SEGV signal when running the test blackwhite in folder pngwriter-build with the following command:

./blackwhite crash_SEGV_readfromfile

This is the ASAN information:

libpng error: Read Error
 PNGwriter::read_png_info - ERROR **: This file may be a corrupted PNG file. (setjmp(*png_ptr)->jmpbf) failed).
 PNGwriter::readfromfile - ERROR **: Error opening file AddressSanitizer:DEADLYSIGNAL
=================================================================
==20801==ERROR: AddressSanitizer: SEGV on unknown address 0x0200856caffc (pc 0x00000047ec5e bp 0x7ffd5b4ffef0 sp 0x7ffd5b4ff680 T0)
==20801==The signal is caused by a READ memory access.
    #0 0x47ec5d in AddressIsPoisoned /home/fouzhe/llvm/llvm/projects/compiler-rt/lib/asan/asan_mapping.h:344
    #1 0x47ec5d in QuickCheckForUnpoisonedRegion /home/fouzhe/llvm/llvm/projects/compiler-rt/lib/asan/asan_interceptors_memintrinsics.h:32
    #2 0x47ec5d in __interceptor_strlen.part.32 /home/fouzhe/llvm/llvm/projects/compiler-rt/lib/asan/../sanitizer_common/sanitizer_common_interceptors.inc:343
    #3 0x5404e3 in std::char_traits<char>::length(char const*) /usr/lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/bits/char_traits.h:267:16
    #4 0x5404e3 in std::basic_ostream<char, std::char_traits<char> >& std::operator<< <std::char_traits<char> >(std::basic_ostream<char, std::char_traits<char> >&, char const*) /usr/lib/gcc/x86_64-linux-gnu/5.4.0/../../../../include/c++/5.4.0/ostream:562
    #5 0x5404e3 in pngwriter::readfromfile(char*) /home/fouzhe/my_fuzz/tests/pngwriter/src/pngwriter.cc:1313
    #6 0x51ee9a in main /home/fouzhe/my_fuzz/tests/pngwriter/tests/blackwhite.cc:23:11
    #7 0x7f23bea4382f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #8 0x41e498 in _start (/home/fouzhe/my_fuzz/tests/pngwriter/pngwriter-build/blackwhite+0x41e498)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /home/fouzhe/llvm/llvm/projects/compiler-rt/lib/asan/asan_mapping.h:344 in AddressIsPoisoned
==20801==ABORTING
@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

Thank you for the report and thank you for fuzzying PNGwriter!

Just for reference, can you attach the exact build command you used for AddressSanitizer as well (CMake line)?

@fouzhe
Copy link
Author

fouzhe commented Jul 11, 2018

I use this command before cmake:

export CFLAGS="-g -fsanitize=address" CXXFLAGS="-g -fsanitize=address"

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

Thanks!
Which version of libpng did you use? I am surprised that it reaches line 1313 after the error in read_png_info is raised.

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

I am trying to reproduce the error with your file and clang 6.
What are you passing as char * name to the readfromfile call? Are you hooking that one directly? If yes, is this c-string zero-delimited?

I am using libpng 1.6.34 and have freetype support as well (2.8.1).

In my case, it just aborts reading after check_if_png:

./blackwhite crash_SEGV_readfromfile 1>/dev/null

 PNGwriter::check_if_png - ERROR **: File /crash_SEGV_readfromfile does not appear to be a valid PNG file. png_check_sig() failed.
 PNGwriter::readfromfile - ERROR **: Error opening file /crash_SEGV_readfromfile. This may not be a valid png file. (check_if_png() failed).

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

Looks to me like the string name passing is the one causing the trouble, not the test file.
Should be solvable with #95.

An other issue in readfromfile for the blackwhite (or any read) example is, that a user can not check if the file is valid after the call readfromfile() that might have errored. That means subsequent calls to the pngwriter instance might not be allowed.

@fouzhe
Copy link
Author

fouzhe commented Jul 11, 2018

I use libpng 1.6 .
Did you use AddressSanitizer?

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

Ah, can reproduce now. Checking!
I messed up the download of the file, sorry!

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

Hm, still slightly different but at least crashing with an address violation.

Contrary to the offical docs, calling png_destroy_read_struct on an invalid file seems not to be a sane libpng operation:

./blackwhite crash_SEGV_readfromfile 
libpng error: Read Error
AddressSanitizer:DEADLYSIGNAL
=================================================================
==268==ERROR: AddressSanitizer: SEGV on unknown address 0x0014000000b4 (pc 0x7faa7722e133 bp 0x001400000014 sp 0x7ffd036ad590 T0)
==268==The signal is caused by a READ memory access.
    #0 0x7faa7722e132 in png_free_data (/usr/lib/x86_64-linux-gnu/libpng16.so.16+0x5132)
    #1 0x7faa7722e743 in png_destroy_info_struct (/usr/lib/x86_64-linux-gnu/libpng16.so.16+0x5743)
    #2 0x7faa77239d14 in png_destroy_read_struct (/usr/lib/x86_64-linux-gnu/libpng16.so.16+0x10d14)
    #3 0x52d54a in pngwriter::read_png_info(_IO_FILE*, png_struct_def**, png_info_def**) /pngwriter-0.7.0/src/pngwriter.cc:1494:2
    #4 0x52c683 in pngwriter::readfromfile(char*) /pngwriter-0.7.0/src/pngwriter.cc:1311:8
    #5 0x52dd3c in pngwriter::readfromfile(char const*) /pngwriter-0.7.0/src/pngwriter.cc:1431:10
    #6 0x51b39e in main /pngwriter-0.7.0/tests/blackwhite.cc:23:11
    #7 0x7faa75a19b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #8 0x41e799 in _start (/pngwriter-0.7.0/build/blackwhite+0x41e799)

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

Probably it's the missing error handling of the call to png_read_image

@fouzhe
Copy link
Author

fouzhe commented Jul 11, 2018

Is this a bug of libpng?

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

I think it's in PNGwriter already.

libpng has a nifty way of error handling. Something like C++ exceptions but in C with jumps.
png_read_image throws such an error on your file, but PNGwriter does not properly handle it and does continue to operate on an invalid state.

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

Although, it could also be an error in png_destroy_read_struct directly in libpng...
We call that as recommended on error-handling cleanup but it looks like this is causing the problem.

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

Full stack with libpng 1.6.34:

    #0 0x7f5aee56db24 in png_free_data /libpng-1.6.34/png.c:481:18
    #1 0x7f5aee56da8b in png_destroy_info_struct /libpng-1.6.34/png.c:408:7
    #2 0x7f5aee59d977 in png_destroy_read_struct /libpng-1.6.34/pngread.c:1023:4
    #3 0x52c89a in pngwriter::read_png_info(_IO_FILE*, png_struct_def**, png_info_def**) /pngwriter-0.7.0/src/pngwriter.cc:1494:2
    #4 0x52b9d3 in pngwriter::readfromfile(char*) /pngwriter-0.7.0/src/pngwriter.cc:1311:8

4: png_read_image
3: error handling longjump
2: freeing of allocated memory during error handling via png_destroy_read_struct

   /* libpng 1.6.0: use the API to destroy info structs to ensure consistent
    * behavior.  Prior to 1.6.0 libpng did extra 'info' destruction in this API.
    * The extra was, apparently, unnecessary yet this hides memory leak bugs.
    */
   png_destroy_info_struct(png_ptr, end_info_ptr_ptr);
   png_destroy_info_struct(png_ptr, info_ptr_ptr);  // <--- this one

1: will ultimately call png_free_data(png_ptr, info_ptr, PNG_FREE_ALL, -1);

   if (info_ptr != NULL)
   {
      /* Do this first in case of an error below; if the app implements its own
       * memory management this can lead to png_free calling png_error, which
       * will abort this routine and return control to the app error handler.
       * An infinite loop may result if it then tries to free the same info
       * ptr.
       */
      *info_ptr_ptr = NULL;

      png_free_data(png_ptr, info_ptr, PNG_FREE_ALL, -1);

0: in png_free_data:

#ifdef PNG_TEXT_SUPPORTED
   /* Free text item num or (if num == -1) all text items */
   if (info_ptr->text != NULL &&
       ((mask & PNG_FREE_TEXT) & info_ptr->free_me) != 0)
   {

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

@fouzhe I now think it might actually be a libpng bug...

@ax3l
Copy link
Member

ax3l commented Jul 11, 2018

@fouzhe I tried against libpng 1.5.30 and 1.6.34 now.

The error is newly introduced in libpng 1.6.34 is in both versions and needs to be fixed there. Can you report it with a minimal example upstream? The issue is caused by the recommended error handling in the form of

    if (setjmp(png_jmpbuf(png_ptr)))
    {
       png_destroy_read_struct(&png_ptr, &info_ptr,
           &end_info);
       fclose(fp);
       return (ERROR);
    }

The error seems to be part of the cleanup routines when text support is enabled.

@fouzhe
Copy link
Author

fouzhe commented Jul 12, 2018

Oh,I will try it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants