Stop supporting kUnknown_BmpHeaderType DO NOT MERGE
In SkBmpCodec, if the header size does not match a known header,
stop trying to create an SkCodec. We do not know of any BMPs with
arbitrarily sized headers, so this should not cause any real
regressions.
In addition, this fixes a bug where we attempt to read too much data
from a file. Since we attempt to read the header size in one read,
and a size reported by the "BMP" may be larger than SSIZE_MAX, this
will crash when reading from a file.
Add a test.
Bug: 33651913
Bug: 37627194
Conflicts:
- tests/CodexTest.cpp
- Add the new test without others that have been added since
- Use SkAutoTDelete instead of std::unique_ptr
Change-Id: I0f03acec5981869554b05cd8ccb75d7c87361019
(cherry picked from commit 71eb09f952b7a8c56149e485cf452f03c0e834c6)
diff --git a/resources/invalid_images/b33651913.bmp b/resources/invalid_images/b33651913.bmp
new file mode 100644
index 0000000..5999b56
--- /dev/null
+++ b/resources/invalid_images/b33651913.bmp
Binary files differ
diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp
index 7eb1baf..0aa2dad 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp
@@ -79,6 +79,59 @@
return SkBmpCodec::NewFromStream(stream, true);
}
+// Header size constants
+static const uint32_t kBmpHeaderBytes = 14;
+static const uint32_t kBmpHeaderBytesPlusFour = kBmpHeaderBytes + 4;
+static const uint32_t kBmpOS2V1Bytes = 12;
+static const uint32_t kBmpOS2V2Bytes = 64;
+static const uint32_t kBmpInfoBaseBytes = 16;
+static const uint32_t kBmpInfoV1Bytes = 40;
+static const uint32_t kBmpInfoV2Bytes = 52;
+static const uint32_t kBmpInfoV3Bytes = 56;
+static const uint32_t kBmpInfoV4Bytes = 108;
+static const uint32_t kBmpInfoV5Bytes = 124;
+static const uint32_t kBmpMaskBytes = 12;
+
+static BmpHeaderType get_header_type(size_t infoBytes) {
+ if (infoBytes >= kBmpInfoBaseBytes) {
+ // Check the version of the header
+ switch (infoBytes) {
+ case kBmpInfoV1Bytes:
+ return kInfoV1_BmpHeaderType;
+ case kBmpInfoV2Bytes:
+ return kInfoV2_BmpHeaderType;
+ case kBmpInfoV3Bytes:
+ return kInfoV3_BmpHeaderType;
+ case kBmpInfoV4Bytes:
+ return kInfoV4_BmpHeaderType;
+ case kBmpInfoV5Bytes:
+ return kInfoV5_BmpHeaderType;
+ case 16:
+ case 20:
+ case 24:
+ case 28:
+ case 32:
+ case 36:
+ case 42:
+ case 46:
+ case 48:
+ case 60:
+ case kBmpOS2V2Bytes:
+ return kOS2VX_BmpHeaderType;
+ default:
+ SkCodecPrintf("Error: unknown bmp header format.\n");
+ return kUnknown_BmpHeaderType;
+ }
+ } if (infoBytes >= kBmpOS2V1Bytes) {
+ // The OS2V1 is treated separately because it has a unique format
+ return kOS2V1_BmpHeaderType;
+ } else {
+ // There are no valid bmp headers
+ SkCodecPrintf("Error: second bitmap header size is invalid.\n");
+ return kUnknown_BmpHeaderType;
+ }
+}
+
/*
* Read enough of the stream to initialize the SkBmpCodec. Returns a bool
* representing success or failure. If it returned true, and codecOut was
@@ -86,19 +139,6 @@
* Does *not* take ownership of the passed in SkStream.
*/
bool SkBmpCodec::ReadHeader(SkStream* stream, bool inIco, SkCodec** codecOut) {
- // Header size constants
- static const uint32_t kBmpHeaderBytes = 14;
- static const uint32_t kBmpHeaderBytesPlusFour = kBmpHeaderBytes + 4;
- static const uint32_t kBmpOS2V1Bytes = 12;
- static const uint32_t kBmpOS2V2Bytes = 64;
- static const uint32_t kBmpInfoBaseBytes = 16;
- static const uint32_t kBmpInfoV1Bytes = 40;
- static const uint32_t kBmpInfoV2Bytes = 52;
- static const uint32_t kBmpInfoV3Bytes = 56;
- static const uint32_t kBmpInfoV4Bytes = 108;
- static const uint32_t kBmpInfoV5Bytes = 124;
- static const uint32_t kBmpMaskBytes = 12;
-
// The total bytes in the bmp file
// We only need to use this value for RLE decoding, so we will only
// check that it is valid in the RLE case.
@@ -157,6 +197,12 @@
}
}
+ // Determine image information depending on second header format
+ const BmpHeaderType headerType = get_header_type(infoBytes);
+ if (kUnknown_BmpHeaderType == headerType) {
+ return false;
+ }
+
// We already read the first four bytes of the info header to get the size
const uint32_t infoBytesRemaining = infoBytes - 4;
@@ -182,80 +228,45 @@
// The image width and height
int width, height;
- // Determine image information depending on second header format
- BmpHeaderType headerType;
- if (infoBytes >= kBmpInfoBaseBytes) {
- // Check the version of the header
- switch (infoBytes) {
- case kBmpInfoV1Bytes:
- headerType = kInfoV1_BmpHeaderType;
- break;
- case kBmpInfoV2Bytes:
- headerType = kInfoV2_BmpHeaderType;
- break;
- case kBmpInfoV3Bytes:
- headerType = kInfoV3_BmpHeaderType;
- break;
- case kBmpInfoV4Bytes:
- headerType = kInfoV4_BmpHeaderType;
- break;
- case kBmpInfoV5Bytes:
- headerType = kInfoV5_BmpHeaderType;
- break;
- case 16:
- case 20:
- case 24:
- case 28:
- case 32:
- case 36:
- case 42:
- case 46:
- case 48:
- case 60:
- case kBmpOS2V2Bytes:
- headerType = kOS2VX_BmpHeaderType;
- break;
- default:
- // We do not signal an error here because there is the
- // possibility of new or undocumented bmp header types. Most
- // of the newer versions of bmp headers are similar to and
- // build off of the older versions, so we may still be able to
- // decode the bmp.
- SkCodecPrintf("Warning: unknown bmp header format.\n");
- headerType = kUnknown_BmpHeaderType;
- break;
- }
- // We check the size of the header before entering the if statement.
- // We should not reach this point unless the size is large enough for
- // these required fields.
- SkASSERT(infoBytesRemaining >= 12);
- width = get_int(iBuffer.get(), 0);
- height = get_int(iBuffer.get(), 4);
- bitsPerPixel = get_short(iBuffer.get(), 10);
+ switch (headerType) {
+ case kInfoV1_BmpHeaderType:
+ case kInfoV2_BmpHeaderType:
+ case kInfoV3_BmpHeaderType:
+ case kInfoV4_BmpHeaderType:
+ case kInfoV5_BmpHeaderType:
+ case kOS2VX_BmpHeaderType:
+ // We check the size of the header before entering the if statement.
+ // We should not reach this point unless the size is large enough for
+ // these required fields.
+ SkASSERT(infoBytesRemaining >= 12);
+ width = get_int(iBuffer.get(), 0);
+ height = get_int(iBuffer.get(), 4);
+ bitsPerPixel = get_short(iBuffer.get(), 10);
- // Some versions do not have these fields, so we check before
- // overwriting the default value.
- if (infoBytesRemaining >= 16) {
- compression = get_int(iBuffer.get(), 12);
- if (infoBytesRemaining >= 32) {
- numColors = get_int(iBuffer.get(), 28);
+ // Some versions do not have these fields, so we check before
+ // overwriting the default value.
+ if (infoBytesRemaining >= 16) {
+ compression = get_int(iBuffer.get(), 12);
+ if (infoBytesRemaining >= 32) {
+ numColors = get_int(iBuffer.get(), 28);
+ }
}
- }
- // All of the headers that reach this point, store color table entries
- // using 4 bytes per pixel.
- bytesPerColor = 4;
- } else if (infoBytes >= kBmpOS2V1Bytes) {
- // The OS2V1 is treated separately because it has a unique format
- headerType = kOS2V1_BmpHeaderType;
- width = (int) get_short(iBuffer.get(), 0);
- height = (int) get_short(iBuffer.get(), 2);
- bitsPerPixel = get_short(iBuffer.get(), 6);
- bytesPerColor = 3;
- } else {
- // There are no valid bmp headers
- SkCodecPrintf("Error: second bitmap header size is invalid.\n");
- return false;
+ // All of the headers that reach this point, store color table entries
+ // using 4 bytes per pixel.
+ bytesPerColor = 4;
+ break;
+ case kOS2V1_BmpHeaderType:
+ // The OS2V1 is treated separately because it has a unique format
+ width = (int) get_short(iBuffer.get(), 0);
+ height = (int) get_short(iBuffer.get(), 2);
+ bitsPerPixel = get_short(iBuffer.get(), 6);
+ bytesPerColor = 3;
+ break;
+ case kUnknown_BmpHeaderType:
+ // We'll exit above in this case.
+ SkASSERT(false);
+ return false;
}
// Check for valid dimensions from header
diff --git a/tests/CodexTest.cpp b/tests/CodexTest.cpp
index 342c369..6aa8ed5 100644
--- a/tests/CodexTest.cpp
+++ b/tests/CodexTest.cpp
@@ -1032,3 +1032,19 @@
test_info(r, codec.get(), codec->getInfo(), SkCodec::kIncompleteInput, nullptr);
}
+
+DEF_TEST(Codec_InvalidBmp2, r) {
+ // This file reports a header size that crashes when we try to read this
+ // much directly from a file using SkFILEStream.
+ SkString path = GetResourcePath("invalid_images/b33651913.bmp");
+ SkAutoTDelete<SkFILEStream> stream(new SkFILEStream(path.c_str()));
+ if (!stream->isValid()) {
+ ERRORF(r, "no stream");
+ return;
+ }
+
+ SkAutoTDelete<SkCodec> codec(SkCodec::NewFromStream(stream.release()));
+ // This file is invalid, but more importantly, we did not crash before
+ // reaching here.
+ REPORTER_ASSERT(r, !codec);
+}
