Beef up cfx_globaldata_unittest.cpp.
Make one method public for testing.
Replace method referencing no members with anonymous functions.
Fix recent botch where nType was narrowed to uint8_t but was
still being written as uint16_t.
Fix longstanding bug where we generate an incorrect object
count for the persistent representation when an array is
seen and omitted from the stream as discovered by test.
This would result in an uninitialized read otherwise.
Change-Id: Ic5fea91003500b70dceffb15e5941250c2d40fa5
Reviewed-on: https://pdfium-review.googlesource.com/c/45130
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/fxjs/cfx_globaldata.cpp b/fxjs/cfx_globaldata.cpp
index c6f9e91..120a875 100644
--- a/fxjs/cfx_globaldata.cpp
+++ b/fxjs/cfx_globaldata.cpp
@@ -30,13 +30,59 @@
0x0e, 0xd0, 0x6b, 0xbb, 0xd5, 0x75, 0x55, 0x8b, 0x6e, 0x6b, 0x19, 0xa0,
0xf8, 0x77, 0xd5, 0xa3};
+CFX_GlobalData* g_pInstance = nullptr;
+
// Returns true if non-empty, setting sPropName
bool TrimPropName(ByteString* sPropName) {
sPropName->Trim();
return sPropName->GetLength() != 0;
}
-CFX_GlobalData* g_pInstance = nullptr;
+void MakeNameTypeString(const ByteString& name,
+ CFX_KeyValue::DataType eType,
+ CFX_BinaryBuf* result) {
+ uint32_t dwNameLen = (uint32_t)name.GetLength();
+ result->AppendBlock(&dwNameLen, sizeof(uint32_t));
+ result->AppendString(name);
+
+ uint16_t wType = static_cast<uint16_t>(eType);
+ result->AppendBlock(&wType, sizeof(uint16_t));
+}
+
+bool MakeByteString(const ByteString& name,
+ const CFX_KeyValue& pData,
+ CFX_BinaryBuf* result) {
+ switch (pData.nType) {
+ case CFX_KeyValue::DataType::NUMBER: {
+ MakeNameTypeString(name, pData.nType, result);
+ double dData = pData.dData;
+ result->AppendBlock(&dData, sizeof(double));
+ return true;
+ }
+ case CFX_KeyValue::DataType::BOOLEAN: {
+ MakeNameTypeString(name, pData.nType, result);
+ uint16_t wData = static_cast<uint16_t>(pData.bData);
+ result->AppendBlock(&wData, sizeof(uint16_t));
+ return true;
+ }
+ case CFX_KeyValue::DataType::STRING: {
+ MakeNameTypeString(name, pData.nType, result);
+ uint32_t dwDataLen = (uint32_t)pData.sData.GetLength();
+ result->AppendBlock(&dwDataLen, sizeof(uint32_t));
+ result->AppendString(pData.sData);
+ return true;
+ }
+ case CFX_KeyValue::DataType::NULLOBJ: {
+ MakeNameTypeString(name, pData.nType, result);
+ return true;
+ }
+ // TODO(tsepez): persist these array objects.
+ case CFX_KeyValue::DataType::OBJECT:
+ default:
+ break;
+ }
+ return false;
+}
} // namespace
@@ -208,7 +254,7 @@
return pdfium::CollectionSize<int32_t>(m_arrayGlobalData);
}
-CFX_GlobalData::Element* CFX_GlobalData::GetAt(int index) const {
+CFX_GlobalData::Element* CFX_GlobalData::GetAt(int index) {
if (index < 0 || index >= GetSize())
return nullptr;
return m_arrayGlobalData[index].get();
@@ -323,15 +369,18 @@
uint32_t nCount = 0;
CFX_BinaryBuf sData;
for (const auto& pElement : m_arrayGlobalData) {
- if (pElement->bPersistent) {
- CFX_BinaryBuf sElement;
- MakeByteString(pElement->data.sKey, &pElement->data, sElement);
- if (sData.GetSize() + sElement.GetSize() > kMaxGlobalDataBytes)
- break;
+ if (!pElement->bPersistent)
+ continue;
- sData.AppendBlock(sElement.GetBuffer(), sElement.GetSize());
- nCount++;
- }
+ CFX_BinaryBuf sElement;
+ if (!MakeByteString(pElement->data.sKey, pElement->data, &sElement))
+ continue;
+
+ if (sData.GetSize() + sElement.GetSize() > kMaxGlobalDataBytes)
+ break;
+
+ sData.AppendBlock(sElement.GetBuffer(), sElement.GetSize());
+ nCount++;
}
CFX_BinaryBuf sFile;
@@ -350,49 +399,6 @@
m_pDelegate->StoreBuffer({sFile.GetBuffer(), sFile.GetSize()});
}
-void CFX_GlobalData::MakeByteString(const ByteString& name,
- CFX_KeyValue* pData,
- CFX_BinaryBuf& sData) {
- switch (pData->nType) {
- case CFX_KeyValue::DataType::NUMBER: {
- uint32_t dwNameLen = (uint32_t)name.GetLength();
- sData.AppendBlock(&dwNameLen, sizeof(uint32_t));
- sData.AppendString(name);
- sData.AppendBlock(&pData->nType, sizeof(uint16_t));
-
- double dData = pData->dData;
- sData.AppendBlock(&dData, sizeof(double));
- } break;
- case CFX_KeyValue::DataType::BOOLEAN: {
- uint32_t dwNameLen = (uint32_t)name.GetLength();
- sData.AppendBlock(&dwNameLen, sizeof(uint32_t));
- sData.AppendString(name);
- sData.AppendBlock(&pData->nType, sizeof(uint16_t));
-
- uint16_t wData = (uint16_t)pData->bData;
- sData.AppendBlock(&wData, sizeof(uint16_t));
- } break;
- case CFX_KeyValue::DataType::STRING: {
- uint32_t dwNameLen = (uint32_t)name.GetLength();
- sData.AppendBlock(&dwNameLen, sizeof(uint32_t));
- sData.AppendString(name);
- sData.AppendBlock(&pData->nType, sizeof(uint16_t));
-
- uint32_t dwDataLen = (uint32_t)pData->sData.GetLength();
- sData.AppendBlock(&dwDataLen, sizeof(uint32_t));
- sData.AppendString(pData->sData);
- } break;
- case CFX_KeyValue::DataType::NULLOBJ: {
- uint32_t dwNameLen = (uint32_t)name.GetLength();
- sData.AppendBlock(&dwNameLen, sizeof(uint32_t));
- sData.AppendString(name);
- sData.AppendBlock(&pData->nType, sizeof(uint32_t));
- } break;
- default:
- break;
- }
-}
-
CFX_GlobalData::Element::Element() = default;
CFX_GlobalData::Element::~Element() = default;
diff --git a/fxjs/cfx_globaldata.h b/fxjs/cfx_globaldata.h
index b567017..3a79a6e 100644
--- a/fxjs/cfx_globaldata.h
+++ b/fxjs/cfx_globaldata.h
@@ -50,7 +50,10 @@
bool DeleteGlobalVariable(ByteString propname);
int32_t GetSize() const;
- Element* GetAt(int index) const;
+ Element* GetAt(int index);
+
+ // Exposed for testing.
+ Element* GetGlobalVariable(const ByteString& sPropname);
private:
using iterator = std::vector<std::unique_ptr<Element>>::iterator;
@@ -63,7 +66,6 @@
void LoadGlobalPersistentVariablesFromBuffer(pdfium::span<uint8_t> buffer);
void SaveGlobalPersisitentVariables();
- Element* GetGlobalVariable(const ByteString& sPropname);
iterator FindGlobalVariable(const ByteString& sPropname);
const_iterator FindGlobalVariable(const ByteString& sPropname) const;
@@ -73,9 +75,6 @@
void WriteFileBuffer(const wchar_t* sFilePath,
const char* pBuffer,
int32_t nLength);
- void MakeByteString(const ByteString& name,
- CFX_KeyValue* pData,
- CFX_BinaryBuf& sData);
size_t m_RefCount = 0;
UnownedPtr<Delegate> const m_pDelegate;
diff --git a/fxjs/cfx_globaldata_unittest.cpp b/fxjs/cfx_globaldata_unittest.cpp
index a4574ce..52a13dc 100644
--- a/fxjs/cfx_globaldata_unittest.cpp
+++ b/fxjs/cfx_globaldata_unittest.cpp
@@ -4,15 +4,18 @@
#include "fxjs/cfx_globaldata.h"
+#include <utility>
#include <vector>
#include "testing/gtest/include/gtest/gtest.h"
#include "testing/test_support.h"
+#include "third_party/base/ptr_util.h"
namespace {
class TestDelegate : public CFX_GlobalData::Delegate {
public:
+ TestDelegate() = default;
~TestDelegate() override {}
bool StoreBuffer(pdfium::span<const uint8_t> buffer) override {
@@ -31,13 +34,39 @@
} // namespace
+TEST(CFXGlobalData, GetSafety) {
+ CFX_GlobalData* pInstance = CFX_GlobalData::GetRetainedInstance(nullptr);
+ EXPECT_EQ(nullptr, pInstance->GetGlobalVariable("nonesuch"));
+ EXPECT_EQ(nullptr, pInstance->GetAt(-1));
+ EXPECT_EQ(nullptr, pInstance->GetAt(0));
+ EXPECT_EQ(nullptr, pInstance->GetAt(1));
+
+ pInstance->SetGlobalVariableNumber("double", 2.0);
+ pInstance->SetGlobalVariableString("string", "clams");
+
+ EXPECT_EQ(nullptr, pInstance->GetGlobalVariable("nonesuch"));
+ EXPECT_EQ(nullptr, pInstance->GetAt(-1));
+ EXPECT_EQ(pInstance->GetGlobalVariable("double"), pInstance->GetAt(0));
+ EXPECT_EQ(pInstance->GetGlobalVariable("string"), pInstance->GetAt(1));
+ EXPECT_EQ(nullptr, pInstance->GetAt(2));
+
+ ASSERT_TRUE(pInstance->Release());
+}
+
TEST(CFXGlobalData, StoreReload) {
TestDelegate delegate;
+ CFX_GlobalArray array;
CFX_GlobalData* pInstance = CFX_GlobalData::GetRetainedInstance(&delegate);
pInstance->SetGlobalVariableNumber("double", 2.0);
pInstance->SetGlobalVariableString("string", "clams");
+ pInstance->SetGlobalVariableBoolean("boolean", true);
+ pInstance->SetGlobalVariableNull("null");
+ pInstance->SetGlobalVariableObject("array", std::move(array));
pInstance->SetGlobalVariablePersistent("double", true);
pInstance->SetGlobalVariablePersistent("string", true);
+ pInstance->SetGlobalVariablePersistent("boolean", true);
+ pInstance->SetGlobalVariablePersistent("null", true);
+ pInstance->SetGlobalVariablePersistent("array", true);
ASSERT_TRUE(pInstance->Release());
pInstance = CFX_GlobalData::GetRetainedInstance(&delegate);
@@ -52,5 +81,85 @@
EXPECT_EQ("string", element->data.sKey);
EXPECT_EQ(CFX_KeyValue::DataType::STRING, element->data.nType);
EXPECT_EQ("clams", element->data.sData);
+
+ element = pInstance->GetAt(2);
+ ASSERT_TRUE(element);
+ EXPECT_EQ("boolean", element->data.sKey);
+ EXPECT_EQ(CFX_KeyValue::DataType::BOOLEAN, element->data.nType);
+ EXPECT_EQ(true, element->data.bData);
+
+ element = pInstance->GetAt(3);
+ ASSERT_TRUE(element);
+ EXPECT_EQ("null", element->data.sKey);
+ EXPECT_EQ(CFX_KeyValue::DataType::NULLOBJ, element->data.nType);
+
+ // TODO(tsepez): arrays don't get persisted.
+ element = pInstance->GetAt(4);
+ ASSERT_FALSE(element);
+
+ ASSERT_TRUE(pInstance->Release());
+}
+
+TEST(CFXGlobalData, ResetValues) {
+ CFX_GlobalData* pInstance = CFX_GlobalData::GetRetainedInstance(nullptr);
+ pInstance->SetGlobalVariableString("double", "bogus!!!");
+ pInstance->SetGlobalVariableString("string", "bogus!!!");
+ pInstance->SetGlobalVariableString("boolean", "bogus!!!");
+ pInstance->SetGlobalVariableString("null", "bogus!!!");
+
+ pInstance->SetGlobalVariableNumber("double", 2.0);
+ pInstance->SetGlobalVariableString("string", "clams");
+ pInstance->SetGlobalVariableBoolean("boolean", true);
+ pInstance->SetGlobalVariableNull("null");
+
+ auto* element = pInstance->GetAt(0);
+ ASSERT_TRUE(element);
+ EXPECT_EQ("double", element->data.sKey);
+ EXPECT_EQ(CFX_KeyValue::DataType::NUMBER, element->data.nType);
+ EXPECT_EQ(2.0, element->data.dData);
+
+ element = pInstance->GetAt(1);
+ ASSERT_TRUE(element);
+ EXPECT_EQ("string", element->data.sKey);
+ EXPECT_EQ(CFX_KeyValue::DataType::STRING, element->data.nType);
+ EXPECT_EQ("clams", element->data.sData);
+
+ element = pInstance->GetAt(2);
+ ASSERT_TRUE(element);
+ EXPECT_EQ("boolean", element->data.sKey);
+ EXPECT_EQ(CFX_KeyValue::DataType::BOOLEAN, element->data.nType);
+ EXPECT_EQ(true, element->data.bData);
+
+ element = pInstance->GetAt(3);
+ ASSERT_TRUE(element);
+ EXPECT_EQ("null", element->data.sKey);
+ EXPECT_EQ(CFX_KeyValue::DataType::NULLOBJ, element->data.nType);
+
+ ASSERT_TRUE(pInstance->Release());
+}
+
+TEST(CFXGlobalData, DeleteValues) {
+ CFX_GlobalData* pInstance = CFX_GlobalData::GetRetainedInstance(nullptr);
+ pInstance->SetGlobalVariableNumber("double", 2.0);
+ pInstance->SetGlobalVariableString("string", "clams");
+ pInstance->SetGlobalVariableBoolean("boolean", true);
+ pInstance->SetGlobalVariableNull("null");
+ EXPECT_EQ(4, pInstance->GetSize());
+
+ pInstance->DeleteGlobalVariable("nonesuch");
+ EXPECT_EQ(4, pInstance->GetSize());
+
+ pInstance->DeleteGlobalVariable("boolean");
+ EXPECT_EQ(3, pInstance->GetSize());
+
+ pInstance->DeleteGlobalVariable("string");
+ EXPECT_EQ(2, pInstance->GetSize());
+
+ pInstance->DeleteGlobalVariable("double");
+ EXPECT_EQ(1, pInstance->GetSize());
+
+ pInstance->DeleteGlobalVariable("null");
+ EXPECT_EQ(0, pInstance->GetSize());
+
ASSERT_TRUE(pInstance->Release());
}
