From ed1c58049f0c164969946b6ad0ff06d952ab1949 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Tue, 19 Jun 2018 16:23:52 +0000 Subject: 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 Commit-Queue: Tom Sepez --- BUILD.gn | 1 + core/fpdfapi/page/cpdf_page.cpp | 14 ----- core/fpdfapi/page/cpdf_pageobjectholder.cpp | 15 +++++ .../page/cpdf_pageobjectholder_unittest.cpp | 66 ++++++++++++++++++++++ core/fxcrt/fx_extension.h | 17 ++++++ core/fxcrt/fx_extension_unittest.cpp | 30 ++++++++++ 6 files changed, 129 insertions(+), 14 deletions(-) create mode 100644 core/fpdfapi/page/cpdf_pageobjectholder_unittest.cpp diff --git a/BUILD.gn b/BUILD.gn index 132f3c28e5..934601bfd1 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -2864,6 +2864,7 @@ test("pdfium_unittests") { "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 4cd58dddaa..7746253564 100644 --- a/core/fpdfapi/page/cpdf_page.cpp +++ b/core/fpdfapi/page/cpdf_page.cpp @@ -219,17 +219,3 @@ int CPDF_Page::GetPageRotation() const { 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 3eb88ec9c0..67f68c7c42 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 0000000000..ddad795fea --- /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 + +#include "testing/gtest/include/gtest/gtest.h" + +// See https://crbug.com/852273 +TEST(CPDFPageObjectHolder, GraphicsDataAsKey) { + const float fMin = std::numeric_limits::min(); + const float fMax = std::numeric_limits::max(); + const float fInf = std::numeric_limits::infinity(); + const float fNan = std::numeric_limits::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 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 cef943fc99..dcdd64e1fc 100644 --- a/core/fxcrt/fx_extension.h +++ b/core/fxcrt/fx_extension.h @@ -8,6 +8,7 @@ #define CORE_FXCRT_FX_EXTENSION_H_ #include +#include #include #include @@ -92,4 +93,20 @@ void FXSYS_IntToFourHexChars(uint16_t c, char* buf); size_t FXSYS_ToUTF16BE(uint32_t unicode, char* buf); +// Strict order over floating types where NaNs may be present. +template +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 +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 39f26b534b..d84071b83a 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 + #include "testing/gtest/include/gtest/gtest.h" TEST(fxcrt, FXSYS_HexCharToInt) { @@ -134,3 +137,30 @@ TEST(fxcrt, FXSYS_wcstof) { FXSYS_wcstof(L"99999999999999999", 17, &used_len)); EXPECT_EQ(17, used_len); } + +TEST(fxcrt, FXSYS_SafeOps) { + const float fMin = std::numeric_limits::min(); + const float fMax = std::numeric_limits::max(); + const float fInf = std::numeric_limits::infinity(); + const float fNan = std::numeric_limits::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; + } + } + } +} -- cgit v1.2.3