Avoid a CHECK failure inside CXFA_AttachNodeList::Insert().
Make sure the node to insert before, when it exists, is actually a
member of the CXFA_AttachNodeList. Give CXFA_List::Insert() a return
value, so when the validation fails, the caller can indicate failure.
BUG=pdfium:1263
Change-Id: Ia11f94a2407281694080d42cddbcc1c076bde5c9
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/52177
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/fxjs/BUILD.gn b/fxjs/BUILD.gn
index ae3befc..30648f9 100644
--- a/fxjs/BUILD.gn
+++ b/fxjs/BUILD.gn
@@ -252,6 +252,7 @@
"xfa/cfxjse_formcalc_context_embeddertest.cpp",
"xfa/cfxjse_value_embeddertest.cpp",
"xfa/cjx_hostpseudomodel_embeddertest.cpp",
+ "xfa/cjx_list_embeddertest.cpp",
]
deps += [ "../xfa/fxfa" ]
}
diff --git a/fxjs/xfa/cjx_list.cpp b/fxjs/xfa/cjx_list.cpp
index 28eb215..7c0cf23 100644
--- a/fxjs/xfa/cjx_list.cpp
+++ b/fxjs/xfa/cjx_list.cpp
@@ -62,7 +62,9 @@
auto* pBeforeNode =
ToNode(static_cast<CFXJSE_Engine*>(runtime)->ToXFAObject(params[1]));
- GetXFAList()->Insert(pNewNode, pBeforeNode);
+ if (!GetXFAList()->Insert(pNewNode, pBeforeNode))
+ return CJS_Result::Failure(JSMessage::kValueError);
+
return CJS_Result::Success();
}
diff --git a/fxjs/xfa/cjx_list_embeddertest.cpp b/fxjs/xfa/cjx_list_embeddertest.cpp
new file mode 100644
index 0000000..02450dc
--- /dev/null
+++ b/fxjs/xfa/cjx_list_embeddertest.cpp
@@ -0,0 +1,15 @@
+// Copyright 2019 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 "testing/gtest/include/gtest/gtest.h"
+#include "testing/xfa_js_embedder_test.h"
+
+class CJX_ListEmbedderTest : public XFAJSEmbedderTest {};
+
+// Should not crash.
+TEST_F(CJX_ListEmbedderTest, BUG_1263) {
+ ASSERT_TRUE(OpenDocument("simple_xfa.pdf"));
+
+ EXPECT_FALSE(Execute("nodes.insert($form,$)"));
+}
diff --git a/xfa/fxfa/parser/cxfa_arraynodelist.cpp b/xfa/fxfa/parser/cxfa_arraynodelist.cpp
index 704ad54..db36dca 100644
--- a/xfa/fxfa/parser/cxfa_arraynodelist.cpp
+++ b/xfa/fxfa/parser/cxfa_arraynodelist.cpp
@@ -27,14 +27,18 @@
m_array.push_back(pNode);
}
-void CXFA_ArrayNodeList::Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) {
+bool CXFA_ArrayNodeList::Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) {
if (!pBeforeNode) {
m_array.push_back(pNewNode);
- } else {
- auto it = std::find(m_array.begin(), m_array.end(), pBeforeNode);
- if (it != m_array.end())
- m_array.insert(it, pNewNode);
+ return true;
}
+
+ auto it = std::find(m_array.begin(), m_array.end(), pBeforeNode);
+ if (it == m_array.end())
+ return false;
+
+ m_array.insert(it, pNewNode);
+ return true;
}
void CXFA_ArrayNodeList::Remove(CXFA_Node* pNode) {
diff --git a/xfa/fxfa/parser/cxfa_arraynodelist.h b/xfa/fxfa/parser/cxfa_arraynodelist.h
index b00b4a8..238c52c 100644
--- a/xfa/fxfa/parser/cxfa_arraynodelist.h
+++ b/xfa/fxfa/parser/cxfa_arraynodelist.h
@@ -19,10 +19,10 @@
explicit CXFA_ArrayNodeList(CXFA_Document* pDocument);
~CXFA_ArrayNodeList() override;
- // From CXFA_TreeList.
+ // CXFA_TreeList:
size_t GetLength() override;
void Append(CXFA_Node* pNode) override;
- void Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) override;
+ bool Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) override;
void Remove(CXFA_Node* pNode) override;
CXFA_Node* Item(size_t iIndex) override;
diff --git a/xfa/fxfa/parser/cxfa_attachnodelist.cpp b/xfa/fxfa/parser/cxfa_attachnodelist.cpp
index e7d4c1d..8fc67cb1 100644
--- a/xfa/fxfa/parser/cxfa_attachnodelist.cpp
+++ b/xfa/fxfa/parser/cxfa_attachnodelist.cpp
@@ -29,12 +29,16 @@
m_pAttachNode->InsertChild(pNode, nullptr);
}
-void CXFA_AttachNodeList::Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) {
+bool CXFA_AttachNodeList::Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) {
+ if (pBeforeNode && pBeforeNode->GetParent() != m_pAttachNode)
+ return false;
+
CXFA_Node* pParent = pNewNode->GetParent();
if (pParent)
pParent->RemoveChild(pNewNode, true);
m_pAttachNode->InsertChild(pNewNode, pBeforeNode);
+ return true;
}
void CXFA_AttachNodeList::Remove(CXFA_Node* pNode) {
diff --git a/xfa/fxfa/parser/cxfa_attachnodelist.h b/xfa/fxfa/parser/cxfa_attachnodelist.h
index a8248e6..2b20d70 100644
--- a/xfa/fxfa/parser/cxfa_attachnodelist.h
+++ b/xfa/fxfa/parser/cxfa_attachnodelist.h
@@ -17,10 +17,10 @@
CXFA_AttachNodeList(CXFA_Document* pDocument, CXFA_Node* pAttachNode);
~CXFA_AttachNodeList() override;
- // From CXFA_TreeList.
+ // CXFA_TreeList:
size_t GetLength() override;
void Append(CXFA_Node* pNode) override;
- void Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) override;
+ bool Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) override;
void Remove(CXFA_Node* pNode) override;
CXFA_Node* Item(size_t iIndex) override;
diff --git a/xfa/fxfa/parser/cxfa_list.h b/xfa/fxfa/parser/cxfa_list.h
index bfe6508..c114801 100644
--- a/xfa/fxfa/parser/cxfa_list.h
+++ b/xfa/fxfa/parser/cxfa_list.h
@@ -19,7 +19,7 @@
virtual size_t GetLength() = 0;
virtual void Append(CXFA_Node* pNode) = 0;
- virtual void Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) = 0;
+ virtual bool Insert(CXFA_Node* pNewNode, CXFA_Node* pBeforeNode) = 0;
virtual void Remove(CXFA_Node* pNode) = 0;
virtual CXFA_Node* Item(size_t iIndex) = 0;
