Refactor SearchNameNodeByNameInternal() Refactor SearchNameNodeByNameInternal() by performing the following: 1. Update the function comment to make it clearer and more modern. 2. Combine `ppFind` and `pFindIndex` into a single struct, so that callers are forced to pass in both or neither. Update callers to use this struct. Bug: 372523840 Change-Id: I78d2257242f7afbfc22c75fee9fdb8e97caae7b7 Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/128230 Reviewed-by: Lei Zhang <thestig@chromium.org> Commit-Queue: Andy Phan <andyphan@chromium.org> Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfdoc/cpdf_nametree.cpp b/core/fpdfdoc/cpdf_nametree.cpp index ecc64cf..adfb890 100644 --- a/core/fpdfdoc/cpdf_nametree.cpp +++ b/core/fpdfdoc/cpdf_nametree.cpp
@@ -24,6 +24,12 @@ constexpr int kNameTreeMaxRecursion = 32; +// The node where a new name should be inserted at `index`. +struct NodeToInsert { + RetainPtr<CPDF_Array> names; + int index = -1; +}; + std::pair<WideString, WideString> GetNodeLimitsAndSanitize( CPDF_Array* pLimits) { DCHECK(pLimits); @@ -190,19 +196,17 @@ return false; } -// Search for |csName| in the tree with root |pNode|. If successful, return the -// value that |csName| points to; |nIndex| will be the index of |csName|, -// |ppFind| will be the leaf array that |csName| is found in, and |pFindIndex| -// will be the index of |csName| in |ppFind|. If |csName| is not found, |ppFind| -// will be the leaf array that |csName| should be added to, and |pFindIndex| -// will be the index that it should be added at. +// Search for `csName` in the tree with root `pNode`. If successful, return the +// value that `csName` points to; `nIndex` will be the index of `csName`. If +// `node_to_insert` is non-nullptr, then on a successful search, it will contain +// the leaf array where `csName` is found in at the provided index, or on +// failure, the array that `csName` should be inserted in at the provided index. RetainPtr<const CPDF_Object> SearchNameNodeByNameInternal( const RetainPtr<CPDF_Dictionary>& pNode, const WideString& csName, int nLevel, size_t* nIndex, - RetainPtr<CPDF_Array>* ppFind, - int* pFindIndex, + NodeToInsert* node_to_insert, std::set<uint32_t>* seen_obj_nums) { if (nLevel > kNameTreeMaxRecursion) return nullptr; @@ -223,10 +227,10 @@ // Skip this node if the name to look for is greater than its higher limit, // and the node itself is a leaf node. if (csName.Compare(csRight) > 0 && pNames) { - if (ppFind) - *ppFind = pNames; - if (pFindIndex) - *pFindIndex = fxcrt::CollectionSize<int32_t>(*pNames) / 2 - 1; + if (node_to_insert) { + node_to_insert->names = pNames; + node_to_insert->index = fxcrt::CollectionSize<int32_t>(*pNames) / 2 - 1; + } return nullptr; } } @@ -239,10 +243,10 @@ int32_t iCompare = csValue.Compare(csName); if (iCompare > 0) break; - if (ppFind) - *ppFind = pNames; - if (pFindIndex) - *pFindIndex = pdfium::checked_cast<int32_t>(i); + if (node_to_insert) { + node_to_insert->names = pNames; + node_to_insert->index = pdfium::checked_cast<int32_t>(i); + } if (iCompare < 0) continue; @@ -264,7 +268,7 @@ continue; RetainPtr<const CPDF_Object> pFound = SearchNameNodeByNameInternal( - pKid, csName, nLevel + 1, nIndex, ppFind, pFindIndex, seen_obj_nums); + pKid, csName, nLevel + 1, nIndex, node_to_insert, seen_obj_nums); if (pFound) return pFound; } @@ -276,12 +280,11 @@ RetainPtr<const CPDF_Object> SearchNameNodeByName( const RetainPtr<CPDF_Dictionary>& pNode, const WideString& csName, - RetainPtr<CPDF_Array>* ppFind, - int* pFindIndex) { + NodeToInsert* node_to_insert) { size_t nIndex = 0; std::set<uint32_t> seen_obj_nums; - return SearchNameNodeByNameInternal(pNode, csName, 0, &nIndex, ppFind, - pFindIndex, &seen_obj_nums); + return SearchNameNodeByNameInternal(pNode, csName, 0, &nIndex, node_to_insert, + &seen_obj_nums); } struct IndexSearchResult { @@ -484,25 +487,26 @@ bool CPDF_NameTree::AddValueAndName(RetainPtr<CPDF_Object> pObj, const WideString& name) { - RetainPtr<CPDF_Array> pFind; - int nFindIndex = -1; + NodeToInsert node_to_insert; // Handle the corner case where the root node is empty. i.e. No kids and no // names. In which case, just insert into it and skip all the searches. RetainPtr<CPDF_Array> pNames = m_pRoot->GetMutableArrayFor("Names"); - if (pNames && pNames->IsEmpty() && !m_pRoot->GetArrayFor("Kids")) - pFind = pNames; - - if (!pFind) { - // Fail if the tree already contains this name or if the tree is too deep. - if (SearchNameNodeByName(m_pRoot, name, &pFind, &nFindIndex)) - return false; + if (pNames && pNames->IsEmpty() && !m_pRoot->GetArrayFor("Kids")) { + node_to_insert.names = pNames; } - // If the returned |pFind| is a nullptr, then |name| is smaller than all - // existing entries in the tree, and we did not find a leaf array to place - // |name| into. We instead will find the leftmost leaf array in which to place - // |name| and |pObj|. - if (!pFind) { + if (!node_to_insert.names) { + // Fail if the tree already contains this name or if the tree is too deep. + if (SearchNameNodeByName(m_pRoot, name, &node_to_insert)) { + return false; + } + } + + // If the returned `node_to_insert.names` is a nullptr, then `name` is smaller + // than all existing entries in the tree, and we did not find a leaf array to + // place `name` into. We instead will find the leftmost leaf array in which to + // place `name` and `pObj`. + if (!node_to_insert.names) { std::optional<IndexSearchResult> result = SearchNameNodeByIndex(m_pRoot.Get(), 0); if (!result.has_value()) { @@ -510,21 +514,22 @@ return false; } - pFind = result.value().container; - DCHECK(pFind); + node_to_insert.names = result.value().container; + DCHECK(node_to_insert.names); } // Insert the name and the object into the leaf array found. Note that the // insertion position is right after the key-value pair returned by |index|. - size_t nNameIndex = (nFindIndex + 1) * 2; + size_t nNameIndex = (node_to_insert.index + 1) * 2; size_t nValueIndex = nNameIndex + 1; - pFind->InsertNewAt<CPDF_String>(nNameIndex, name.AsStringView()); - pFind->InsertAt(nValueIndex, std::move(pObj)); + node_to_insert.names->InsertNewAt<CPDF_String>(nNameIndex, + name.AsStringView()); + node_to_insert.names->InsertAt(nValueIndex, std::move(pObj)); // Expand the limits that the newly added name is under, if the name falls // outside of the limits of its leaf array or any arrays above it. std::vector<CPDF_Array*> all_limits = - GetNodeAncestorsLimits(m_pRoot, pFind.Get()); + GetNodeAncestorsLimits(m_pRoot, node_to_insert.names.Get()); for (auto* pLimits : all_limits) { if (!pLimits) continue; @@ -573,7 +578,7 @@ RetainPtr<const CPDF_Object> CPDF_NameTree::LookupValue( const WideString& csName) const { - return SearchNameNodeByName(m_pRoot, csName, nullptr, nullptr); + return SearchNameNodeByName(m_pRoot, csName, nullptr); } RetainPtr<const CPDF_Array> CPDF_NameTree::LookupNewStyleNamedDest(