Rework libpng error handling to catch version mismatch errors as well

doc/changelog-plugins.dox

@@ -264,7 +264,8 @@ namespace Magnum {
 -   @relativeref{Trade,PngImporter} and @relativeref{Trade,PngImageConverter}
     no longer asserts if the patch version of libpng changed, such as when a
     system package was upgraded from 1.6.37 to 1.6.38. If the major or minor
-    version changes, it's still an assert.
+    version changes, it's now a runtime error, produced by libpng itself,
+    instead of an assert.
 
 @subsection changelog-plugins-latest-deprecated Deprecated APIs
 

package/ci/appveyor.yml

@@ -2,32 +2,32 @@ clone_depth: 1
 
 environment:
   matrix:
-  - TARGET: desktop
-    COMPILER: msvc
-    APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
-    APPVEYOR_JOB_NAME: windows-msvc2015
-  - TARGET: desktop
-    COMPILER: msvc
-    APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017
-    APPVEYOR_JOB_NAME: windows-msvc2017
+  # - TARGET: desktop
+  #   COMPILER: msvc
+  #   APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2015
+  #   APPVEYOR_JOB_NAME: windows-msvc2015
+  # - TARGET: desktop
+  #   COMPILER: msvc
+  #   APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017
+  #   APPVEYOR_JOB_NAME: windows-msvc2017
   - TARGET: desktop
     COMPILER: msvc
     APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
     APPVEYOR_JOB_NAME: windows-msvc2019
-  - TARGET: desktop
-    COMPILER: msvc
-    APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022
-    APPVEYOR_JOB_NAME: windows-msvc2022
-  - TARGET: desktop
-    COMPILER: msvc
-    APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
-    APPVEYOR_JOB_NAME: windows-static-msvc2019
-    BUILD_STATIC: ON
-  - TARGET: desktop
-    COMPILER: msvc
-    APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022
-    APPVEYOR_JOB_NAME: windows-static-msvc2022
-    BUILD_STATIC: ON
+  # - TARGET: desktop
+  #   COMPILER: msvc
+  #   APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022
+  #   APPVEYOR_JOB_NAME: windows-msvc2022
+  # - TARGET: desktop
+  #   COMPILER: msvc
+  #   APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
+  #   APPVEYOR_JOB_NAME: windows-static-msvc2019
+  #   BUILD_STATIC: ON
+  # - TARGET: desktop
+  #   COMPILER: msvc
+  #   APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022
+  #   APPVEYOR_JOB_NAME: windows-static-msvc2022
+  #   BUILD_STATIC: ON
   - TARGET: desktop
     COMPILER: msvc-clang
     APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
@@ -36,29 +36,29 @@ environment:
     COMPILER: msvc-clang
     APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022
     APPVEYOR_JOB_NAME: windows-msvc2022-clang
-  - TARGET: desktop
-    COMPILER: msvc
-    # Same reasoning as in Corrade for /EHsc
-    COMPILER_EXTRA: -DCMAKE_CXX_FLAGS="/permissive- /EHsc" -DMSVC_COMPATIBILITY=OFF
-    APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
-    APPVEYOR_JOB_NAME: windows-conforming-msvc2019
-  - TARGET: desktop
-    COMPILER: msvc
-    # Not playing with fire and using /EHsc on 2022 as well
-    COMPILER_EXTRA: -DCMAKE_CXX_FLAGS="/permissive- /EHsc" -DMSVC_COMPATIBILITY=OFF
-    APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022
-    APPVEYOR_JOB_NAME: windows-conforming-msvc2022
-  - TARGET: desktop
-    COMPILER: mingw
-    APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017
-    APPVEYOR_JOB_NAME: windows-mingw
-  - TARGET: rt
-    # This actually isn't needed anywhere, but without this the %COMPILE:~0,4%
-    # expressions cause a "The syntax of the command is incorrect." sometimes
-    # but not in all cases. What the fuck, cmd.exe.
-    COMPILER: msvc
-    APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017
-    APPVEYOR_JOB_NAME: windows-rt-msvc2017
+  # - TARGET: desktop
+  #   COMPILER: msvc
+  #   # Same reasoning as in Corrade for /EHsc
+  #   COMPILER_EXTRA: -DCMAKE_CXX_FLAGS="/permissive- /EHsc" -DMSVC_COMPATIBILITY=OFF
+  #   APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2019
+  #   APPVEYOR_JOB_NAME: windows-conforming-msvc2019
+  # - TARGET: desktop
+  #   COMPILER: msvc
+  #   # Not playing with fire and using /EHsc on 2022 as well
+  #   COMPILER_EXTRA: -DCMAKE_CXX_FLAGS="/permissive- /EHsc" -DMSVC_COMPATIBILITY=OFF
+  #   APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2022
+  #   APPVEYOR_JOB_NAME: windows-conforming-msvc2022
+  # - TARGET: desktop
+  #   COMPILER: mingw
+  #   APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017
+  #   APPVEYOR_JOB_NAME: windows-mingw
+  # - TARGET: rt
+  #   # This actually isn't needed anywhere, but without this the %COMPILE:~0,4%
+  #   # expressions cause a "The syntax of the command is incorrect." sometimes
+  #   # but not in all cases. What the fuck, cmd.exe.
+  #   COMPILER: msvc
+  #   APPVEYOR_BUILD_WORKER_IMAGE: Visual Studio 2017
+  #   APPVEYOR_JOB_NAME: windows-rt-msvc2017
 
 install:
 # Ninja. `cinst ninja` started 503ing in late November 2019 and wasn't really
@@ -91,7 +91,8 @@ install:
 
 # libpng, for MSVC 2022, 2019 and clang-cl. MSVC 2017 seems to work well with
 # the 2019 build, MinGW doesn't work at all due to ABI issues.
-- IF "%TARGET%" == "desktop" IF "%COMPILER:~0,4%" == "msvc" appveyor DownloadFile https://ci.magnum.graphics/libpng-1.6.39-windows-2019.zip && 7z x libpng-1.6.39-windows-2019.zip -o%APPVEYOR_BUILD_FOLDER%\deps
+- IF "%TARGET%" == "desktop" IF "%COMPILER%" == "msvc" appveyor DownloadFile https://ci.magnum.graphics/libpng-1.6.39-windows-2019.zip && 7z x libpng-1.6.39-windows-2019.zip -o%APPVEYOR_BUILD_FOLDER%\deps
+- IF "%TARGET%" == "desktop" IF "%COMPILER%" == "msvc-clang" appveyor DownloadFile https://ci.magnum.graphics/libpng-1.6.39-windows-2019-clang-cl.zip && 7z x libpng-1.6.39-windows-2019-clang-cl.zip -o%APPVEYOR_BUILD_FOLDER%\deps
 
 # OpenAL
 - IF NOT "%TARGET%" == "rt" IF NOT EXIST %APPVEYOR_BUILD_FOLDER%\openal-soft-1.19.1-bin.zip appveyor DownloadFile https://openal-soft.org/openal-binaries/openal-soft-1.19.1-bin.zip

src/MagnumPlugins/PngImageConverter/PngImageConverter.cpp

@@ -41,6 +41,7 @@
 #include <csetjmp>
 #include <Corrade/Containers/Array.h>
 #include <Corrade/Containers/Optional.h>
+#include <Corrade/Containers/ScopeGuard.h>
 #include <Corrade/Containers/String.h>
 #include <Magnum/ImageView.h>
 #include <Magnum/PixelFormat.h>
@@ -109,42 +110,82 @@ Containers::Optional<Containers::Array<char>> PngImageConverter::doConvertToData
             return {};
     }
 
-    png_structp file = png_create_write_struct(PNG_LIBPNG_VER_STRING, nullptr, nullptr, nullptr);
-    /** @todo this will assert if the PNG major/minor version doesn't match,
-        with "libpng warning: Application built with libpng-1.7.0 but running
-        with 1.6.38" being printed to stdout, the proper fix is to set error
-        callbacks directly in the png_create_write_struct() call */
-    CORRADE_INTERNAL_ASSERT(file);
-    png_infop info = png_create_info_struct(file);
-    CORRADE_INTERNAL_ASSERT(info);
+    /* Structures for writing the file. To avoid leaks, everything that needs
+       to be destructed when exiting the function has to be defined *before*
+       the setjmp() call below. */
+    struct PngState {
+        png_structp file;
+        png_infop info;
+    } png{};
+    /** @todo a capturing ScopeGuard would be nicer :( */
+    Containers::ScopeGuard pngStateGuard{&png, [](PngState* state) {
+        /* Although undocumented, this function gracefully handles the case
+           when the file/info is null, no need to check that explicitly. */
+        png_destroy_write_struct(&state->file, &state->info);
+    }};
     std::string output;
-
-    /* Error handling routine. Since we're replacing the png_default_error()
-       function, we need to call std::longjmp() ourselves -- otherwise the
-       default error handling with stderr printing kicks in. */
-    if(setjmp(png_jmpbuf(file))) {
-        png_destroy_write_struct(&file, &info);
+    /* For exiting out of error callbacks via longjmp. Can't use the builtin
+       png_jmpbuf() because the errors can happen inside
+       png_create_read_struct(), thus even before the png.file gets filled, and
+       thus even before we can call setjmp(png_jmpbuf(png.file)). Yet the
+       official documentation says absolutely nothing about handling such case,
+       F.F.S. */
+    std::jmp_buf jmpbuf;
+    /* We may arrive here from the longjmp from the error callback */
+    if(setjmp(jmpbuf))
         return {};
-    }
-    png_set_error_fn(file, nullptr, [](const png_structp file, const png_const_charp message) {
+
+    /* Create the write structure. Directly set up also error/warning
+       callbacks because if done here and not in a subsequent
+       png_set_error_fn() call, it'll gracefully handle also libpng version
+       mismatch errors. */
+    png.file = png_create_write_struct(PNG_LIBPNG_VER_STRING, &jmpbuf, [](const png_structp file, const png_const_charp message) {
         Error{} << "Trade::PngImageConverter::convertToData(): error:" << message;
-        std::longjmp(png_jmpbuf(file), 1);
-    }, [](png_structp, const png_const_charp message) {
-        Warning{} << "Trade::PngImageConverter::convertToData(): warning:" << message;
+        /* Since we're replacing the png_default_error() function, we need to
+           call std::longjmp() ourselves -- otherwise the default error
+           handling with stderr printing kicks in. See above for why
+           png_jmpbuf() can't be used. */
+        std::longjmp(*static_cast<std::jmp_buf*>(png_get_error_ptr(file)), 1);
+    }, [](const png_structp file, const png_const_charp message) {
+        /* If there's a mismatch in the passed PNG_LIBPNG_VER_STRING major or
+           minor version (but not patch version), png_create_read_struct()
+           returns a nullptr. However, such a *fatal* error is treated as a
+           warning by the library, directed to the warning callback:
+             https://github.com/glennrp/libpng/blob/07b8803110da160b158ebfef872627da6c85cbdf/png.c#L219-L240
+           That may cause great confusion ("why convertToData() returns a
+           NullOpt if it was just a warning??"), so I'm detecting that,
+           annotating it as an error instead, and jumping to the error return
+           so we don't end up with a nullptr `png.file` below.
+
+           Unfortunately this case is annoyingly hard to test automatically, so
+           the following branch is uncovered. */
+        if(Containers::StringView{message}.hasPrefix("Application built with libpng-"_s)) {
+            Error{} << "Trade::PngImageConverter::convertToData(): error:" << message;
+            std::longjmp(*static_cast<std::jmp_buf*>(png_get_error_ptr(file)), 1);
+        } else
+            Warning{} << "Trade::PngImageConverter::convertToData(): warning:" << message;
     });
+    /* If png_create_write_struct() failed, png.file is a nullptr. Apart from
+       that we may arrive here from the longjmp from the error callback. */
+    if(!png.file || setjmp(png_jmpbuf(png.file)))
+        return {};
+
+    png.info = png_create_info_struct(png.file);
+    CORRADE_INTERNAL_ASSERT(png.info);
 
-    png_set_write_fn(file, &output, [](png_structp file, png_bytep data, png_size_t length){
+    /* Set functions for writing */
+    png_set_write_fn(png.file, &output, [](png_structp file, png_bytep data, png_size_t length){
         auto&& output = *reinterpret_cast<std::string*>(png_get_io_ptr(file));
         std::size_t oldSize = output.size();
         output.resize(output.size() + length);
         std::copy_n(data, length, &output[oldSize]);
     }, [](png_structp){});
 
     /* Write header */
-    png_set_IHDR(file, info, image.size().x(), image.size().y(),
+    png_set_IHDR(png.file, png.info, image.size().x(), image.size().y(),
         bitDepth, colorType, PNG_INTERLACE_NONE,
         PNG_COMPRESSION_TYPE_BASE, PNG_FILTER_TYPE_BASE);
-    png_write_info(file, info);
+    png_write_info(png.file, png.info);
 
     /* Get data properties and calculate the initial slice based on subimage
        offset */
@@ -155,20 +196,19 @@ Containers::Optional<Containers::Array<char>> PngImageConverter::doConvertToData
     /* Write rows in reverse order, properly take stride into account */
     if(bitDepth == 8) {
         for(Int y = 0; y != image.size().y(); ++y)
-            png_write_row(file, const_cast<unsigned char*>(data.exceptPrefix((image.size().y() - y - 1)*dataProperties.second.x()).data()));
+            png_write_row(png.file, const_cast<unsigned char*>(data.exceptPrefix((image.size().y() - y - 1)*dataProperties.second.x()).data()));
 
     /* For 16 bit depth we need to swap to big endian */
     } else if(bitDepth == 16) {
         #ifndef CORRADE_TARGET_BIG_ENDIAN
-        png_set_swap(file);
+        png_set_swap(png.file);
         #endif
         for(Int y = 0; y != image.size().y(); ++y) {
-            png_write_row(file, const_cast<unsigned char*>(reinterpret_cast<const unsigned char*>(data.exceptPrefix((image.size().y() - y - 1)*dataProperties.second.x()).data())));
+            png_write_row(png.file, const_cast<unsigned char*>(reinterpret_cast<const unsigned char*>(data.exceptPrefix((image.size().y() - y - 1)*dataProperties.second.x()).data())));
         }
     }
 
-    png_write_end(file, nullptr);
-    png_destroy_write_struct(&file, &info);
+    png_write_end(png.file, nullptr);
 
     /* Copy the string into the output array (I would kill for having std::string::release()) */
     Containers::Array<char> fileData{NoInit, output.size()};