commit 75ff13168ea0fd0c8ac8981722d15cce07fbdcf9
author Lei Zhang <thestig@chromium.org> Wed Sep 13 00:16:28 2023 +0000
committer Pdfium LUCI CQ <pdfium-scoped@luci-project-accounts.iam.gserviceaccount.com> Wed Sep 13 00:16:28 2023 +0000
tree d7bf16029999b8cb6ebcddf436bfbd14d3f22831
parent a836c20b87e15724980fbec73229b7dee531fca8

Reinstate CPDF_FormField DCHECKs and improve SetValue()

Undo a small portion of https://pdfium-review.googlesource.com/111552
so SetItemSelection() and IsItemSelected() get their DCHECKs back.
Instead, fix the issue by changing the caller to avoid tripping the
DCHECKs. As part of this work:

- Use CHECK instead of DCHECK, per modern coding guidelines.
- Add an IsComboOrListField() helper function and use that everywhere.
- Add a HasOptField() helper function and use that as an additional
  guardrail to make sure CPDF_FormField only reads /Opt values when
  appropriate.
- Update FPDFAnnot_IsOptionSelected() to avoid tripping over the /Opt
  checks.

Bug: chromium:1477093
Change-Id: Id774393afcd8e976ffa7f2ae869ceac9c9680b06
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/111670
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>

core/fpdfdoc/cpdf_formfield.cpp

diff --git a/core/fpdfdoc/cpdf_formfield.cpp b/core/fpdfdoc/cpdf_formfield.cpp
index f4a62bd..bfcb797 100644
--- a/core/fpdfdoc/cpdf_formfield.cpp
+++ b/core/fpdfdoc/cpdf_formfield.cpp
@@ -50,6 +50,28 @@
       nLevel + 1);
 }
 
+bool IsComboOrListField(CPDF_FormField::Type type) {
+  switch (type) {
+    case CPDF_FormField::kComboBox:
+    case CPDF_FormField::kListBox:
+      return true;
+    default:
+      return false;
+  }
+}
+
+bool HasOptField(CPDF_FormField::Type type) {
+  switch (type) {
+    case CPDF_FormField::kCheckBox:
+    case CPDF_FormField::kRadioButton:
+    case CPDF_FormField::kComboBox:
+    case CPDF_FormField::kListBox:
+      return true;
+    default:
+      return false;
+  }
+}
+
 }  // namespace
 
 // static
@@ -348,7 +370,7 @@
 bool CPDF_FormField::SetValue(const WideString& value,
                               bool bDefault,
                               NotificationOption notify) {
-  switch (m_Type) {
+  switch (GetType()) {
     case kCheckBox:
     case kRadioButton: {
       SetCheckValue(value, bDefault, notify);
@@ -366,7 +388,13 @@
       ByteString key(bDefault ? pdfium::form_fields::kDV
                               : pdfium::form_fields::kV);
       m_pDict->SetNewFor<CPDF_String>(key, csValue.AsStringView());
-      int iIndex = FindOption(csValue);
+
+      int iIndex;
+      if (GetType() == kComboBox) {
+        iIndex = FindOption(csValue);
+      } else {
+        iIndex = -1;
+      }
       if (iIndex < 0) {
         if (m_Type == kRichText && !bDefault) {
           m_pDict->SetFor(pdfium::form_fields::kRV,
@@ -493,9 +521,7 @@
 }
 
 bool CPDF_FormField::IsItemSelected(int index) const {
-  if (GetType() != kComboBox && GetType() != kListBox) {
-    return false;
-  }
+  CHECK(IsComboOrListField(GetType()));
   if (index < 0 || index >= CountOptions()) {
     return false;
   }
@@ -505,9 +531,7 @@
 }
 
 bool CPDF_FormField::SetItemSelection(int index, NotificationOption notify) {
-  if (GetType() != kComboBox && GetType() != kListBox) {
-    return false;
-  }
+  CHECK(IsComboOrListField(GetType()));
   if (index < 0 || index >= CountOptions()) {
     return false;
   }
@@ -554,7 +578,7 @@
 }
 
 int CPDF_FormField::GetDefaultSelectedItem() const {
-  DCHECK(GetType() == kComboBox || GetType() == kListBox);
+  CHECK(IsComboOrListField(GetType()));
   RetainPtr<const CPDF_Object> pValue = GetDefaultValueObject();
   if (!pValue)
     return -1;
@@ -569,11 +593,13 @@
 }
 
 int CPDF_FormField::CountOptions() const {
+  CHECK(HasOptField(GetType()));
   RetainPtr<const CPDF_Array> pArray = ToArray(GetFieldAttrInternal("Opt"));
   return pArray ? fxcrt::CollectionSize<int>(*pArray) : 0;
 }
 
 WideString CPDF_FormField::GetOptionText(int index, int sub_index) const {
+  CHECK(HasOptField(GetType()));
   RetainPtr<const CPDF_Array> pArray = ToArray(GetFieldAttrInternal("Opt"));
   if (!pArray)
     return WideString();
@@ -776,7 +802,7 @@
 }
 
 bool CPDF_FormField::UseSelectedIndicesObject() const {
-  DCHECK(GetType() == kComboBox || GetType() == kListBox);
+  CHECK(IsComboOrListField(GetType()));
 
   RetainPtr<const CPDF_Object> pSelectedIndicesObject =
       GetSelectedIndicesObject();
@@ -895,13 +921,13 @@
 }
 
 RetainPtr<const CPDF_Object> CPDF_FormField::GetSelectedIndicesObject() const {
-  DCHECK(GetType() == kComboBox || GetType() == kListBox);
+  CHECK(IsComboOrListField(GetType()));
   return GetFieldAttrInternal("I");
 }
 
 RetainPtr<const CPDF_Object> CPDF_FormField::GetValueOrSelectedIndicesObject()
     const {
-  DCHECK(GetType() == kComboBox || GetType() == kListBox);
+  CHECK(IsComboOrListField(GetType()));
   RetainPtr<const CPDF_Object> pValue = GetValueObject();
   return pValue ? pValue : GetSelectedIndicesObject();
 }

fpdfsdk/fpdf_annot.cpp

diff --git a/fpdfsdk/fpdf_annot.cpp b/fpdfsdk/fpdf_annot.cpp
index cb7c96d..0cba33e 100644
--- a/fpdfsdk/fpdf_annot.cpp
+++ b/fpdfsdk/fpdf_annot.cpp
@@ -1300,19 +1300,22 @@
 FPDFAnnot_IsOptionSelected(FPDF_FORMHANDLE handle,
                            FPDF_ANNOTATION annot,
                            int index) {
-  if (index < 0)
+  if (index < 0) {
     return false;
+  }
 
   const CPDF_FormField* form_field = GetFormField(handle, annot);
-  if (!form_field || index >= form_field->CountOptions())
+  if (!form_field) {
     return false;
+  }
 
   if (form_field->GetFieldType() != FormFieldType::kComboBox &&
       form_field->GetFieldType() != FormFieldType::kListBox) {
     return false;
   }
 
-  return form_field->IsItemSelected(index);
+  return index < form_field->CountOptions() &&
+         form_field->IsItemSelected(index);
 }
 
 FPDF_EXPORT FPDF_BOOL FPDF_CALLCONV