Change implementation of CPDF_FormField::IsItemSelected()
The PDF spec indicates that the /V entry of a choice field should take
precendence over the /I entry for identifying the selected options.
Additionally, the /I entry was only introduced in PDF 1.4, meaning that
there can exist PDF choice fields that contain an /V entry and no /I
entry, which is not correctly handled by the current implementation of
the method.
Bug: pdfium:1505
Change-Id: I7646e2f6d69e581ea68c55da11a24b26e26ee45d
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/68475
Commit-Queue: Daniel Hosseinian <dhoss@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfdoc/cpdf_formfield.cpp b/core/fpdfdoc/cpdf_formfield.cpp
index 3e33c79..2d18612 100644
--- a/core/fpdfdoc/cpdf_formfield.cpp
+++ b/core/fpdfdoc/cpdf_formfield.cpp
@@ -6,6 +6,7 @@
#include "core/fpdfdoc/cpdf_formfield.h"
+#include <map>
#include <memory>
#include <set>
#include <utility>
@@ -133,6 +134,7 @@
m_Type = kListBox;
m_bIsMultiSelectListBox = flags & pdfium::form_flags::kChoiceMultiSelect;
}
+ m_bUseSelectedIndices = UseSelectedIndicesObject();
LoadDA();
} else if (type_name == pdfium::form_fields::kSig) {
m_Type = kSign;
@@ -476,34 +478,10 @@
ASSERT(GetType() == kComboBox || GetType() == kListBox);
if (index < 0 || index >= CountOptions())
return false;
- if (IsOptionSelected(index))
- return true;
- WideString opt_value = GetOptionValue(index);
- const CPDF_Object* pValue = GetValueOrSelectedIndicesObject();
- if (!pValue)
- return false;
-
- if (pValue->IsString())
- return pValue->GetUnicodeText() == opt_value;
-
- if (pValue->IsNumber()) {
- if (pValue->GetString().IsEmpty())
- return false;
- return (pValue->GetInteger() == index);
- }
-
- const CPDF_Array* pArray = pValue->AsArray();
- if (!pArray)
- return false;
-
- for (int i = 0; i < CountSelectedOptions(); ++i) {
- if (GetSelectedOptionIndex(i) == index) {
- const CPDF_Object* pDirectObj = pArray->GetDirectObjectAt(i);
- return pDirectObj && pDirectObj->GetUnicodeText() == opt_value;
- }
- }
- return false;
+ // First consider the /I entry if it is valid, then fall back to the /V entry.
+ return m_bUseSelectedIndices ? IsSelectedIndex(index)
+ : IsSelectedOption(GetOptionValue(index));
}
bool CPDF_FormField::SetItemSelection(int index,
@@ -524,6 +502,11 @@
else
SetItemSelectionUnselected(index, opt_value);
+ // UseSelectedIndicesObject() has a non-trivial linearithmic run-time, so run
+ // only if necessary.
+ if (!m_bUseSelectedIndices)
+ m_bUseSelectedIndices = UseSelectedIndicesObject();
+
if (notify == NotificationOption::kNotify)
NotifyListOrComboBoxAfterChange();
return true;
@@ -755,17 +738,40 @@
return pArray->GetIntegerAt(index);
}
-bool CPDF_FormField::IsOptionSelected(int iOptIndex) const {
- const CPDF_Array* pArray = ToArray(GetSelectedIndicesObject());
- if (!pArray)
+bool CPDF_FormField::IsSelectedOption(const WideString& wsOptValue) const {
+ const CPDF_Object* pValueObject = GetValueObject();
+ if (!pValueObject)
return false;
- CPDF_ArrayLocker locker(pArray);
- for (const auto& pObj : locker) {
- if (pObj->GetInteger() == iOptIndex)
- return true;
+ const CPDF_Array* pValueArray = pValueObject->AsArray();
+ if (pValueArray) {
+ CPDF_ArrayLocker locker(pValueArray);
+ for (const auto& pObj : locker) {
+ if (pObj->IsString() && pObj->GetUnicodeText() == wsOptValue)
+ return true;
+ }
}
- return false;
+
+ return pValueObject->IsString() &&
+ pValueObject->GetUnicodeText() == wsOptValue;
+}
+
+bool CPDF_FormField::IsSelectedIndex(int iOptIndex) const {
+ const CPDF_Object* pSelectedIndicesObject = GetSelectedIndicesObject();
+ if (!pSelectedIndicesObject)
+ return false;
+
+ const CPDF_Array* pSelectedIndicesArray = pSelectedIndicesObject->AsArray();
+ if (pSelectedIndicesArray) {
+ CPDF_ArrayLocker locker(pSelectedIndicesArray);
+ for (const auto& pObj : locker) {
+ if (pObj->IsNumber() && pObj->GetInteger() == iOptIndex)
+ return true;
+ }
+ }
+
+ return pSelectedIndicesObject->IsNumber() &&
+ pSelectedIndicesObject->GetInteger() == iOptIndex;
}
bool CPDF_FormField::SelectOption(int iOptIndex,
@@ -822,6 +828,83 @@
return true;
}
+bool CPDF_FormField::UseSelectedIndicesObject() const {
+ ASSERT(GetType() == kComboBox || GetType() == kListBox);
+
+ const CPDF_Object* pSelectedIndicesObject = GetSelectedIndicesObject();
+ if (!pSelectedIndicesObject)
+ return false;
+
+ // If there's not value object, then just use the indices object.
+ const CPDF_Object* pValueObject = GetValueObject();
+ if (!pValueObject)
+ return true;
+
+ // Verify that the selected indices object is either an array or a number and
+ // count the number of indices.
+ size_t selected_indices_size;
+ const CPDF_Array* pSelectedIndicesArray = pSelectedIndicesObject->AsArray();
+ if (pSelectedIndicesArray)
+ selected_indices_size = pSelectedIndicesArray->size();
+ else if (pSelectedIndicesObject->IsNumber())
+ selected_indices_size = 1;
+ else
+ return false;
+
+ // Verify that the number of values is equal to |selected_indices_size|. Then,
+ // count the number of occurances of each of the distinct values in the values
+ // object.
+ std::map<WideString, size_t> values;
+ const CPDF_Array* pValueArray = pValueObject->AsArray();
+ if (pValueArray) {
+ if (pValueArray->size() != selected_indices_size)
+ return false;
+ CPDF_ArrayLocker locker(pValueArray);
+ for (const auto& pObj : locker) {
+ if (pObj->IsString())
+ values[pObj->GetUnicodeText()]++;
+ }
+ } else if (pValueObject->IsString()) {
+ if (selected_indices_size != 1)
+ return false;
+ values[pValueObject->GetUnicodeText()]++;
+ }
+
+ // Validate each index in the selected indices object. Then, verify that items
+ // identified by selected indices entry do not differ from those in the values
+ // entry of the field dictionary.
+ const int num_options = CountOptions();
+ if (pSelectedIndicesArray) {
+ CPDF_ArrayLocker locker(pSelectedIndicesArray);
+ for (const auto& pObj : locker) {
+ if (!pObj->IsNumber())
+ return false;
+
+ int index = pObj->GetInteger();
+ if (index < 0 || index >= num_options)
+ return false;
+
+ WideString wsOptValue = GetOptionValue(index);
+ auto it = values.find(wsOptValue);
+ if (it == values.end())
+ return false;
+
+ it->second--;
+ if (it->second == 0)
+ values.erase(it);
+ }
+
+ return values.empty();
+ }
+
+ ASSERT(pSelectedIndicesObject->IsNumber());
+ int index = pSelectedIndicesObject->GetInteger();
+ if (index < 0 || index >= num_options)
+ return false;
+
+ return pdfium::ContainsKey(values, GetOptionValue(index));
+}
+
void CPDF_FormField::LoadDA() {
CPDF_Dictionary* pFormDict = m_pForm->GetFormDict();
if (!pFormDict)
diff --git a/core/fpdfdoc/cpdf_formfield.h b/core/fpdfdoc/cpdf_formfield.h
index aa51ddc..826f8ec 100644
--- a/core/fpdfdoc/cpdf_formfield.h
+++ b/core/fpdfdoc/cpdf_formfield.h
@@ -130,9 +130,14 @@
int GetTopVisibleIndex() const;
int CountSelectedOptions() const;
int GetSelectedOptionIndex(int index) const;
- bool IsOptionSelected(int iOptIndex) const;
+ bool IsSelectedOption(const WideString& wsOptValue) const;
+ bool IsSelectedIndex(int iOptIndex) const;
bool SelectOption(int iOptIndex, bool bSelected, NotificationOption notify);
+ // Verifies if there is a valid selected indicies (/I) object and whether its
+ // entries are consistent with the value (/V) object.
+ bool UseSelectedIndicesObject() const;
+
float GetFontSize() const { return m_FontSize; }
CPDF_Font* GetFont() const { return m_pFont.Get(); }
@@ -181,6 +186,7 @@
bool m_bNoExport = false;
bool m_bIsMultiSelectListBox = false;
bool m_bIsUnison = false;
+ bool m_bUseSelectedIndices = false;
float m_FontSize = 0;
UnownedPtr<CPDF_InteractiveForm> const m_pForm;
RetainPtr<CPDF_Dictionary> const m_pDict;
diff --git a/core/fpdfdoc/cpdf_formfield_unittest.cpp b/core/fpdfdoc/cpdf_formfield_unittest.cpp
index 2174efb..63346e0 100644
--- a/core/fpdfdoc/cpdf_formfield_unittest.cpp
+++ b/core/fpdfdoc/cpdf_formfield_unittest.cpp
@@ -43,6 +43,7 @@
void TestMultiselectFieldDict(RetainPtr<CPDF_Array> opt_array,
RetainPtr<CPDF_Object> values,
RetainPtr<CPDF_Object> selected_indices,
+ bool expected_use_indices,
const std::vector<int>& expected_indices,
const std::vector<int>& excluded_indices) {
auto form_dict = pdfium::MakeRetain<CPDF_Dictionary>();
@@ -59,6 +60,7 @@
CPDF_TestEmptyDocument doc;
CPDF_InteractiveForm form(&doc);
CPDF_FormField form_field(&form, form_dict.Get());
+ EXPECT_EQ(expected_use_indices, form_field.UseSelectedIndicesObject());
for (int i = 0; i < form_field.CountOptions(); i++) {
const bool expected_selected = pdfium::ContainsValue(expected_indices, i);
EXPECT_EQ(expected_selected, form_field.IsItemSelected(i));
@@ -123,7 +125,8 @@
std::vector<int> expected_indices;
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, /*values=*/nullptr,
- /*selected_indices=*/nullptr, expected_indices,
+ /*selected_indices=*/nullptr,
+ /*expected_use_indices=*/false, expected_indices,
excluded_indices);
}
{
@@ -132,7 +135,8 @@
std::vector<int> expected_indices{2};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, /*selected_indices=*/nullptr,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) object is just an invalid string.
@@ -140,17 +144,18 @@
std::vector<int> expected_indices;
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, /*selected_indices=*/nullptr,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) object is an array with one object.
auto values = pdfium::MakeRetain<CPDF_Array>();
values->AppendNew<CPDF_String>(L"Beta");
- // TODO(bug_1505): Should be selected at index 1.
- std::vector<int> expected_indices;
+ std::vector<int> expected_indices{1};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, /*selected_indices=*/nullptr,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) object is an array with one invalid object.
@@ -159,18 +164,19 @@
std::vector<int> expected_indices;
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, /*selected_indices=*/nullptr,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) object is an array with multiple objects.
auto values = pdfium::MakeRetain<CPDF_Array>();
values->AppendNew<CPDF_String>(L"Beta");
values->AppendNew<CPDF_String>(L"Epsilon");
- // TODO(bug_1505): Should be selected at index 1 and index 4.
- std::vector<int> expected_indices;
+ std::vector<int> expected_indices{1, 4};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, /*selected_indices=*/nullptr,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) object is an array with multiple objects with one invalid.
@@ -178,11 +184,11 @@
values->AppendNew<CPDF_String>(L"Beta");
values->AppendNew<CPDF_String>(L"Epsilon");
values->AppendNew<CPDF_String>(L"Omega");
- // TODO(bug_1505): Should be selected at index 1 and index 4.
- std::vector<int> expected_indices;
+ std::vector<int> expected_indices{1, 4};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, /*selected_indices=*/nullptr,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Selected indices (/I) object is just a number.
@@ -190,7 +196,8 @@
std::vector<int> expected_indices{3};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, /*values=*/nullptr, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/true, expected_indices,
+ excluded_indices);
}
{
// Selected indices (/I) object is just an invalid number.
@@ -198,7 +205,8 @@
std::vector<int> expected_indices;
std::vector<int> excluded_indices{-1, 5, 26};
TestMultiselectFieldDict(opt_array, /*values=*/nullptr, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/true, expected_indices,
+ excluded_indices);
}
{
// Selected indices (/I) object is an array with one object.
@@ -207,7 +215,8 @@
std::vector<int> expected_indices{0};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, /*values=*/nullptr, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/true, expected_indices,
+ excluded_indices);
}
{
// Selected indices (/I) object is an array with multiple objects.
@@ -218,7 +227,8 @@
std::vector<int> expected_indices{0, 2, 3};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, /*values=*/nullptr, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/true, expected_indices,
+ excluded_indices);
}
{
// Selected indices (/I) object is an array with multiple objects and some
@@ -233,7 +243,8 @@
std::vector<int> expected_indices{0, 2, 3};
std::vector<int> excluded_indices{-5, -1, 5, 12, 42};
TestMultiselectFieldDict(opt_array, /*values=*/nullptr, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/true, expected_indices,
+ excluded_indices);
}
{
// Values (/V) or Selected Indices (/I) objects conflict with different
@@ -245,11 +256,11 @@
selected_indices->AppendNew<CPDF_Number>(0);
selected_indices->AppendNew<CPDF_Number>(2);
selected_indices->AppendNew<CPDF_Number>(3);
- // TODO(bug_1505): Should be selected at index 1 and index 4.
- std::vector<int> expected_indices{0, 2, 3};
+ std::vector<int> expected_indices{1, 4};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) or Selected Indices (/I) objects conflict with same lengths.
@@ -259,11 +270,11 @@
auto selected_indices = pdfium::MakeRetain<CPDF_Array>();
selected_indices->AppendNew<CPDF_Number>(2);
selected_indices->AppendNew<CPDF_Number>(3);
- // TODO(bug_1505): Should be selected at index 0 and index 4.
- std::vector<int> expected_indices{2, 3};
+ std::vector<int> expected_indices{0, 4};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) or Selected Indices (/I) objects conflict with values being
@@ -278,7 +289,8 @@
std::vector<int> expected_indices{1, 4};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) or Selected Indices (/I) objects conflict with selected
@@ -293,7 +305,8 @@
std::vector<int> expected_indices{1, 4};
std::vector<int> excluded_indices{-1, 5, 26};
TestMultiselectFieldDict(opt_array, values, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) or Selected Indices (/I) objects conflict with both being
@@ -307,11 +320,11 @@
selected_indices->AppendNew<CPDF_Number>(2);
selected_indices->AppendNew<CPDF_Number>(3);
selected_indices->AppendNew<CPDF_Number>(26);
- // TODO(bug_1505): Should be selected at index 1 and index 4.
- std::vector<int> expected_indices{0, 2, 3};
+ std::vector<int> expected_indices{1, 4};
std::vector<int> excluded_indices{-1, 5, 26};
TestMultiselectFieldDict(opt_array, values, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
{
// Values (/V) or Selected Indices (/I) objects conflict with each not being
@@ -321,6 +334,7 @@
std::vector<int> expected_indices{2};
std::vector<int> excluded_indices{-1, 5};
TestMultiselectFieldDict(opt_array, values, selected_indices,
- expected_indices, excluded_indices);
+ /*expected_use_indices=*/false, expected_indices,
+ excluded_indices);
}
}
diff --git a/fpdfsdk/fpdf_formfill_embeddertest.cpp b/fpdfsdk/fpdf_formfill_embeddertest.cpp
index a12e3b0..e5ab09f 100644
--- a/fpdfsdk/fpdf_formfill_embeddertest.cpp
+++ b/fpdfsdk/fpdf_formfill_embeddertest.cpp
@@ -2838,8 +2838,7 @@
// opening.
FocusOnMultiSelectMultipleValuesForm();
for (int i = 0; i < 5; i++) {
- // TODO(bug_1505): Should be selected at index 2 and index 4.
- bool expected = false;
+ bool expected = (i == 2 || i == 4);
CheckIsIndexSelected(i, expected);
}
}
@@ -2849,8 +2848,7 @@
// opening.
FocusOnMultiSelectMultipleMismatchForm();
for (int i = 0; i < 5; i++) {
- // TODO(bug_1505): Should be selected at index 0 and index 2.
- bool expected = (i == 1 || i == 3 || i == 4);
+ bool expected = (i == 0 || i == 2);
CheckIsIndexSelected(i, expected);
}
}
