From e852277905d5f14e0fdd1dd860e694fad2389aa5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Vladim=C3=ADr=20Vondru=C5=A1?= <mosra@centrum.cz>
Date: Wed, 21 Oct 2020 12:14:35 +0200
Subject: [PATCH] AnyImageImporter: fix signature printing in the error
 message.

And also test various potential false positives, which shouldn't be
detected as given format.

*Damn*, I need some utility class for printing this, this is
unsustainable.
---
 .../AnyImageImporter/AnyImageImporter.cpp     | 14 ++++---
 .../Test/AnyImageImporterTest.cpp             | 37 +++++++++++++++----
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp b/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp
index 4079dc39d..008d4be2e 100644
--- a/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp
+++ b/src/MagnumPlugins/AnyImageImporter/AnyImageImporter.cpp
@@ -30,6 +30,7 @@
 #include <Corrade/PluginManager/PluginMetadata.h>
 #include <Corrade/Utility/Assert.h>
 #include <Corrade/Utility/DebugStl.h>
+#include <Corrade/Utility/FormatStl.h>
 #include <Corrade/Utility/String.h>
 
 #include "Magnum/Trade/ImageData.h"
@@ -196,11 +197,14 @@ void AnyImageImporter::doOpenData(Containers::ArrayView<const char> data) {
         Error{} << "Trade::AnyImageImporter::openData(): file is empty";
         return;
     } else {
-        std::uint32_t signature = data[0] << 24;
-        if(data.size() > 1) signature |= data[1] << 16;
-        if(data.size() > 2) signature |= data[2] << 8;
-        if(data.size() > 3) signature |= data[3];
-        Error{} << "Trade::AnyImageImporter::openData(): cannot determine the format from signature" << reinterpret_cast<void*>(signature);
+        /* FFS so much casting to avoid implicit sign extension ruining
+           everything */
+        UnsignedInt signature = UnsignedInt(UnsignedByte(data[0])) << 24;
+        if(data.size() > 1) signature |= UnsignedInt(UnsignedByte(data[1])) << 16;
+        if(data.size() > 2) signature |= UnsignedInt(UnsignedByte(data[2])) << 8;
+        if(data.size() > 3) signature |= UnsignedInt(UnsignedByte(data[3]));
+        /* If there's less than four bytes, cut the rest away */
+        Error{} << "Trade::AnyImageImporter::openData(): cannot determine the format from signature 0x" << Debug::nospace << Utility::formatString("{:.8x}", signature).substr(0, data.size() < 4 ? data.size()*2 : std::string::npos);
         return;
     }
 
diff --git a/src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp b/src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp
index 57d62c5a0..f6db661cb 100644
--- a/src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp
+++ b/src/MagnumPlugins/AnyImageImporter/Test/AnyImageImporterTest.cpp
@@ -25,6 +25,7 @@
 
 #include <sstream>
 #include <Corrade/Containers/Optional.h>
+#include <Corrade/Containers/StringView.h>
 #include <Corrade/PluginManager/Manager.h>
 #include <Corrade/TestSuite/Tester.h>
 #include <Corrade/Utility/Directory.h>
@@ -97,6 +98,24 @@ constexpr struct {
     /* Not testing everything, just the most important ones */
 };
 
+using namespace Containers::Literals;
+
+const struct {
+    const char* name;
+    Containers::StringView data;
+    const char* signature;
+} DetectUnknownData[]{
+    {"something random", "\x25\x3a\x00\x56 blablabla"_s, "253a0056"},
+    /* There was a bug where the error message shifted a signed value,
+       poisoning the output. It also was throwing away leading zero bytes. */
+    {"leading zeros, negative char", "\x00\xff\x00\xff"_s, "00ff00ff"},
+    {"just one byte", "\x33"_s, "33"},
+    {"just one zero byte", "\x00"_s, "00"},
+    {"DDS, but no space", "DDS!"_s, "44445321"},
+    {"TIFF, but too short", "II\x2a"_s, "49492a"},
+    {"TIFF, but no zero byte", "MM\xff\x2a"_s, "4d4dff2a"}
+};
+
 AnyImageImporterTest::AnyImageImporterTest() {
     addInstancedTests({&AnyImageImporterTest::load},
         Containers::arraySize(LoadData));
@@ -104,9 +123,12 @@ AnyImageImporterTest::AnyImageImporterTest() {
     addInstancedTests({&AnyImageImporterTest::detect},
         Containers::arraySize(DetectData));
 
-    addTests({&AnyImageImporterTest::unknownExtension,
-              &AnyImageImporterTest::unknownSignature,
-              &AnyImageImporterTest::emptyData});
+    addTests({&AnyImageImporterTest::unknownExtension});
+
+    addInstancedTests({&AnyImageImporterTest::unknownSignature},
+        Containers::arraySize(DetectUnknownData));
+
+    addTests({&AnyImageImporterTest::emptyData});
 
     addInstancedTests({&AnyImageImporterTest::verbose},
         Containers::arraySize(LoadData));
@@ -178,15 +200,16 @@ void AnyImageImporterTest::unknownExtension() {
 }
 
 void AnyImageImporterTest::unknownSignature() {
+    auto&& data = DetectUnknownData[testCaseInstanceId()];
+    setTestCaseDescription(data.name);
+
     std::ostringstream output;
     Error redirectError{&output};
 
-    constexpr const char data[]{ 0x25, 0x3a };
-
     Containers::Pointer<AbstractImporter> importer = _manager.instantiate("AnyImageImporter");
-    CORRADE_VERIFY(!importer->openData(data));
+    CORRADE_VERIFY(!importer->openData(data.data));
 
-    CORRADE_COMPARE(output.str(), "Trade::AnyImageImporter::openData(): cannot determine the format from signature 0x253a0000\n");
+    CORRADE_COMPARE(output.str(), Utility::formatString("Trade::AnyImageImporter::openData(): cannot determine the format from signature 0x{}\n", data.signature));
 }
 
 void AnyImageImporterTest::emptyData() {