Speculative fix for segv destroying CPDF_PageObjectHolder::m_GraphicsMap
We speculate that the bug that makes the comparison operator irreflexive
might be the cause of the segv on windows production code, should a
NaN sneak into the GraphicsData struct. In any event, should this
happen, the tree won't be correct with some nodes erroneously replaced.
Add a test which fails prior to the patch, but alas does not elicit
the segv.
Also move operator<() methods to .cpp file corresponding to .h file
in which they are delcared.
Bug: 852273
Change-Id: Ib7929881e7ffbed8b09f6e2c9fb7898cbde58946
Reviewed-on: https://pdfium-review.googlesource.com/35171
Reviewed-by: dsinclair <dsinclair@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/BUILD.gn b/BUILD.gn
index 132f3c2..934601b 100644
--- a/BUILD.gn
+++ b/BUILD.gn
@@ -2864,6 +2864,7 @@
"core/fpdfapi/font/cpdf_cmapparser_unittest.cpp",
"core/fpdfapi/font/cpdf_tounicodemap_unittest.cpp",
"core/fpdfapi/page/cpdf_devicecs_unittest.cpp",
+ "core/fpdfapi/page/cpdf_pageobjectholder_unittest.cpp",
"core/fpdfapi/page/cpdf_psengine_unittest.cpp",
"core/fpdfapi/page/cpdf_streamcontentparser_unittest.cpp",
"core/fpdfapi/page/cpdf_streamparser_unittest.cpp",
diff --git a/core/fpdfapi/page/cpdf_page.cpp b/core/fpdfapi/page/cpdf_page.cpp
index 4cd58dd..7746253 100644
--- a/core/fpdfapi/page/cpdf_page.cpp
+++ b/core/fpdfapi/page/cpdf_page.cpp
@@ -219,17 +219,3 @@
int rotate = pRotate ? (pRotate->GetInteger() / 90) % 4 : 0;
return (rotate < 0) ? (rotate + 4) : rotate;
}
-
-bool GraphicsData::operator<(const GraphicsData& other) const {
- if (fillAlpha != other.fillAlpha)
- return fillAlpha < other.fillAlpha;
- if (strokeAlpha != other.strokeAlpha)
- return strokeAlpha < other.strokeAlpha;
- return blendType < other.blendType;
-}
-
-bool FontData::operator<(const FontData& other) const {
- if (baseFont != other.baseFont)
- return baseFont < other.baseFont;
- return type < other.type;
-}
diff --git a/core/fpdfapi/page/cpdf_pageobjectholder.cpp b/core/fpdfapi/page/cpdf_pageobjectholder.cpp
index 3eb88ec..67f68c7 100644
--- a/core/fpdfapi/page/cpdf_pageobjectholder.cpp
+++ b/core/fpdfapi/page/cpdf_pageobjectholder.cpp
@@ -15,6 +15,21 @@
#include "core/fpdfapi/page/cpdf_pageobject.h"
#include "core/fpdfapi/parser/cpdf_dictionary.h"
#include "core/fpdfapi/parser/cpdf_document.h"
+#include "core/fxcrt/fx_extension.h"
+
+bool GraphicsData::operator<(const GraphicsData& other) const {
+ if (!FXSYS_SafeEQ(fillAlpha, other.fillAlpha))
+ return FXSYS_SafeLT(fillAlpha, other.fillAlpha);
+ if (!FXSYS_SafeEQ(strokeAlpha, other.strokeAlpha))
+ return FXSYS_SafeLT(strokeAlpha, other.strokeAlpha);
+ return blendType < other.blendType;
+}
+
+bool FontData::operator<(const FontData& other) const {
+ if (baseFont != other.baseFont)
+ return baseFont < other.baseFont;
+ return type < other.type;
+}
CPDF_PageObjectHolder::CPDF_PageObjectHolder(CPDF_Document* pDoc,
CPDF_Dictionary* pDict)
diff --git a/core/fpdfapi/page/cpdf_pageobjectholder_unittest.cpp b/core/fpdfapi/page/cpdf_pageobjectholder_unittest.cpp
new file mode 100644
index 0000000..ddad795
--- /dev/null
+++ b/core/fpdfapi/page/cpdf_pageobjectholder_unittest.cpp
@@ -0,0 +1,66 @@
+// Copyright 2018 PDFium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "core/fpdfapi/page/cpdf_pageobjectholder.h"
+
+#include <limits>
+
+#include "testing/gtest/include/gtest/gtest.h"
+
+// See https://crbug.com/852273
+TEST(CPDFPageObjectHolder, GraphicsDataAsKey) {
+ const float fMin = std::numeric_limits<float>::min();
+ const float fMax = std::numeric_limits<float>::max();
+ const float fInf = std::numeric_limits<float>::infinity();
+ const float fNan = std::numeric_limits<float>::quiet_NaN();
+
+ // Verify self-comparisions.
+ for (float c1 : {fMin, 1.0f, 2.0f, fMax, fInf, fNan}) {
+ for (float c2 : {fMin, 1.0f, 2.0f, fMax, fInf, fNan}) {
+ for (int c3 : {1, 2})
+ EXPECT_FALSE(GraphicsData({c1, c2, c3}) < GraphicsData({c1, c2, c3}));
+ }
+ }
+
+ std::map<GraphicsData, int> graphics_map;
+
+ // Insert in reverse index permuted order.
+ size_t x = 0;
+ for (int c3 : {2, 1}) {
+ for (float c2 : {fNan, fInf, fMax, 2.0f, 1.0f, fMin}) {
+ for (float c1 : {fNan, fInf, fMax, 2.0f, 1.0f, fMin}) {
+ graphics_map[{c1, c2, c3}] = x++;
+ }
+ }
+ }
+ EXPECT_EQ(72u, x);
+ EXPECT_EQ(72u, graphics_map.size());
+
+ // clang-format off
+ const int expected[72] = {
+ 71, 35, 65, 29, 59, 23, 53, 17, 47, 11, 41, 5,
+ 70, 34, 64, 28, 58, 22, 52, 16, 46, 10, 40, 4,
+ 69, 33, 63, 27, 57, 21, 51, 15, 45, 9, 39, 3,
+ 68, 32, 62, 26, 56, 20, 50, 14, 44, 8, 38, 2,
+ 67, 31, 61, 25, 55, 19, 49, 13, 43, 7, 37, 1,
+ 66, 30, 60, 24, 54, 18, 48, 12, 42, 6, 36, 0
+ };
+ // clang-format on
+
+ x = 0;
+ for (const auto& item : graphics_map) {
+ EXPECT_EQ(expected[x], item.second) << " for position " << x;
+ ++x;
+ }
+ EXPECT_EQ(72u, x);
+
+ // Erase in forward index permuted order.
+ for (int c3 : {1, 2}) {
+ for (float c2 : {fMin, 1.0f, 2.0f, fMax, fInf, fNan}) {
+ for (float c1 : {fMin, 1.0f, 2.0f, fMax, fInf, fNan})
+ graphics_map.erase({c1, c2, c3});
+ }
+ }
+ EXPECT_EQ(0u, graphics_map.size());
+}
diff --git a/core/fxcrt/fx_extension.h b/core/fxcrt/fx_extension.h
index cef943f..dcdd64e 100644
--- a/core/fxcrt/fx_extension.h
+++ b/core/fxcrt/fx_extension.h
@@ -8,6 +8,7 @@
#define CORE_FXCRT_FX_EXTENSION_H_
#include <cctype>
+#include <cmath>
#include <cwctype>
#include <memory>
@@ -92,4 +93,20 @@
size_t FXSYS_ToUTF16BE(uint32_t unicode, char* buf);
+// Strict order over floating types where NaNs may be present.
+template <typename T>
+bool FXSYS_SafeEQ(const T& lhs, const T& rhs) {
+ return (std::isnan(lhs) && std::isnan(rhs)) ||
+ (!std::isnan(lhs) && !std::isnan(rhs) && lhs == rhs);
+}
+
+template <typename T>
+bool FXSYS_SafeLT(const T& lhs, const T& rhs) {
+ if (std::isnan(lhs) && std::isnan(rhs))
+ return false;
+ if (std::isnan(lhs) || std::isnan(rhs))
+ return std::isnan(lhs) < std::isnan(rhs);
+ return lhs < rhs;
+}
+
#endif // CORE_FXCRT_FX_EXTENSION_H_
diff --git a/core/fxcrt/fx_extension_unittest.cpp b/core/fxcrt/fx_extension_unittest.cpp
index 39f26b5..d84071b 100644
--- a/core/fxcrt/fx_extension_unittest.cpp
+++ b/core/fxcrt/fx_extension_unittest.cpp
@@ -3,6 +3,9 @@
// found in the LICENSE file.
#include "core/fxcrt/fx_extension.h"
+
+#include <limits>
+
#include "testing/gtest/include/gtest/gtest.h"
TEST(fxcrt, FXSYS_HexCharToInt) {
@@ -134,3 +137,30 @@
FXSYS_wcstof(L"99999999999999999", 17, &used_len));
EXPECT_EQ(17, used_len);
}
+
+TEST(fxcrt, FXSYS_SafeOps) {
+ const float fMin = std::numeric_limits<float>::min();
+ const float fMax = std::numeric_limits<float>::max();
+ const float fInf = std::numeric_limits<float>::infinity();
+ const float fNan = std::numeric_limits<float>::quiet_NaN();
+ const float ascending[] = {fMin, 1.0f, 2.0f, fMax, fInf, fNan};
+
+ for (size_t i = 0; i < FX_ArraySize(ascending); ++i) {
+ for (size_t j = 0; j < FX_ArraySize(ascending); ++j) {
+ if (i == j) {
+ EXPECT_TRUE(FXSYS_SafeEQ(ascending[i], ascending[j]))
+ << " at " << i << " " << j;
+ } else {
+ EXPECT_FALSE(FXSYS_SafeEQ(ascending[i], ascending[j]))
+ << " at " << i << " " << j;
+ }
+ if (i < j) {
+ EXPECT_TRUE(FXSYS_SafeLT(ascending[i], ascending[j]))
+ << " at " << i << " " << j;
+ } else {
+ EXPECT_FALSE(FXSYS_SafeLT(ascending[i], ascending[j]))
+ << " at " << i << " " << j;
+ }
+ }
+ }
+}
