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

Fixed a crash when reading PNG files with an invalid header. #385

Merged
merged 1 commit into from
Aug 26, 2019

Conversation

macmade
Copy link
Contributor

@macmade macmade commented Aug 26, 2019

Description

Use of BOOST_ASSERT in png_ptr_read_deleter and png_ptr_write_deleter in ‎⁨include/boost/gil/extension/⁨io/png/⁨detail⁩/reader_backend.hpp will produce a crash with PNG files with an invalid header.

In such a case (bad PNG file), we expect a std::ios_base::failure exception (see below).
Unfortunately, the assertions will prevent the exception to be caught by terminating the program.

This hotfix replaces the assertions with conditional checks, so we can get the expected exception handling.

Steps to reproduce

  1. Create a text file with at least 4 characters (like ABCD).
  2. Try to read the file as PNG:
using backend_t   = typename boost::gil::get_reader_backend< std::string, boost::gil::png_tag >::type;
backend_t backend = boost::gil::read_image_info( "path/to/invalid/png", boost::gil::png_tag() );

Explanation

A reader_backend object is created: ‎⁨include/boost/gil/extension/⁨io/png/⁨detail⁩/reader_backend.hpp.

The base class' constructor (png_struct_info_wrapper) has a std::shared_ptr member for the png_ptr_wrapper.
The shared pointer is created with a custom deleter (png_ptr_read_deleter and png_ptr_write_deleter).

So inside the backend constructor, the png_ptr_wrapper is non-null.
Then, still from the constructor, the PNG header is read by calling read_header.

    io_error_if( png_sig_cmp( png_bytep(buf)
                            , png_size_t(0)
                            , PNG_BYTES_TO_CHECK
                            ) != 0
               , "png_check_validity: invalid png image"
               );

So for an invalid header, io_error_if will throw an exception.
This is the expected behavior.

But as the exception is thrown, the backend object gets deallocated.
So is the std::shared_ptr. And as the deleter is called:

static void png_ptr_read_deleter( png_ptr_wrapper* png_ptr )
{
    if( png_ptr )
    {
        BOOST_ASSERT(png_ptr->_struct && png_ptr->_info);

png_ptr is valid, but all fields will be null, as we haven't read the PNG file yet.
Assertion will fail, making the program crash, and thus preventing the expected exception to be caught.

References

Tasklist

  • Ensure all CI builds pass
  • Review and approve

Removed assertions, as they prevent the expected std::ios_base::failure exception to be caught in such a case.
@mloskot
Copy link
Member

mloskot commented Aug 26, 2019

@macmade

Assertion will fail, making the program crash

It should have also stated "only if compiled with -DNDEBUG", no?

Thanks for the report

@mloskot mloskot added cat/annoyance Not a bug, not a feature, but something that should be improved ext/io boost/gil/extension/io/ labels Aug 26, 2019
@mloskot mloskot added this to the Boost 1.72+ milestone Aug 26, 2019
Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for the fix.

@mloskot mloskot merged commit bd00f91 into boostorg:develop Aug 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cat/annoyance Not a bug, not a feature, but something that should be improved ext/io boost/gil/extension/io/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants