Rework CPDF_LinkExtract::CheckWebLink() Convert to size_t and avoid out parameters. Change-Id: I89eaef3fef2cb4225c2063315f55ac26c4057b92 Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/91470 Reviewed-by: Lei Zhang <thestig@chromium.org> Commit-Queue: Tom Sepez <tsepez@chromium.org>
diff --git a/core/fpdftext/cpdf_linkextract.cpp b/core/fpdftext/cpdf_linkextract.cpp index f822f3b..68bca2d 100644 --- a/core/fpdftext/cpdf_linkextract.cpp +++ b/core/fpdftext/cpdf_linkextract.cpp
@@ -112,11 +112,11 @@ void CPDF_LinkExtract::ExtractLinks() { m_LinkArray.clear(); - int start = 0; - int pos = 0; + size_t start = 0; + size_t pos = 0; bool bAfterHyphen = false; bool bLineBreak = false; - const int nTotalChar = m_pTextPage->CountChars(); + const size_t nTotalChar = m_pTextPage->CountChars(); const WideString page_text = m_pTextPage->GetAllPageText(); while (pos < nTotalChar) { const CPDF_TextPage::CharInfo& char_info = m_pTextPage->GetCharInfo(pos); @@ -130,7 +130,7 @@ continue; } - int nCount = pos - start; + size_t nCount = pos - start; if (pos == nTotalChar - 1) { ++nCount; } else if (bAfterHyphen && @@ -163,13 +163,12 @@ // Check for potential web URLs and email addresses. // Ftp address, file system links, data, blob etc. are not checked. if (nCount > 5) { - int32_t nStartOffset; - int32_t nCountOverload; - if (CheckWebLink(&strBeCheck, &nStartOffset, &nCountOverload)) { - m_LinkArray.push_back( - {start + nStartOffset, nCountOverload, strBeCheck}); + auto maybe_link = CheckWebLink(strBeCheck); + if (maybe_link.has_value()) { + maybe_link.value().m_Start += start; + m_LinkArray.push_back(maybe_link.value()); } else if (CheckMailLink(&strBeCheck)) { - m_LinkArray.push_back({start, nCount, strBeCheck}); + m_LinkArray.push_back(Link{start, nCount, strBeCheck}); } } } @@ -177,36 +176,34 @@ } } -bool CPDF_LinkExtract::CheckWebLink(WideString* strBeCheck, - int32_t* nStart, - int32_t* nCount) { +absl::optional<CPDF_LinkExtract::Link> CPDF_LinkExtract::CheckWebLink( + const WideString& strBeCheck) { static const wchar_t kHttpScheme[] = L"http"; static const wchar_t kWWWAddrStart[] = L"www."; const size_t kHttpSchemeLen = FXSYS_len(kHttpScheme); const size_t kWWWAddrStartLen = FXSYS_len(kWWWAddrStart); - WideString str = *strBeCheck; + WideString str = strBeCheck; str.MakeLower(); - size_t len = str.GetLength(); // First, try to find the scheme. auto start = str.Find(kHttpScheme); if (start.has_value()) { size_t off = start.value() + kHttpSchemeLen; // move after "http". - if (len > off + 4) { // At least "://<char>" follows. + if (str.GetLength() > off + 4) { // At least "://<char>" follows. if (str[off] == L's') // "https" scheme is accepted. off++; if (str[off] == L':' && str[off + 1] == L'/' && str[off + 2] == L'/') { off += 3; - size_t end = TrimExternalBracketsFromWebLink(str, start.value(), - str.GetLength() - 1); - end = FindWebLinkEnding(str, off, end); + const size_t end = + FindWebLinkEnding(str, off, + TrimExternalBracketsFromWebLink( + str, start.value(), str.GetLength() - 1)); if (end > off) { // Non-empty host name. - *nStart = start.value(); - *nCount = end - start.value() + 1; - *strBeCheck = strBeCheck->Substr(*nStart, *nCount); - return true; + const size_t nStart = start.value(); + const size_t nCount = end - nStart + 1; + return Link{nStart, nCount, strBeCheck.Substr(nStart, nCount)}; } } } @@ -214,18 +211,23 @@ // When there is no scheme, try to find url starting with "www.". start = str.Find(kWWWAddrStart); - if (start.has_value() && len > start.value() + kWWWAddrStartLen) { - size_t end = TrimExternalBracketsFromWebLink(str, start.value(), - str.GetLength() - 1); - end = FindWebLinkEnding(str, start.value(), end); - if (end > start.value() + kWWWAddrStartLen) { - *nStart = start.value(); - *nCount = end - start.value() + 1; - *strBeCheck = L"http://" + strBeCheck->Substr(*nStart, *nCount); - return true; + if (start.has_value()) { + size_t off = start.value() + kWWWAddrStartLen; + if (str.GetLength() > off) { + const size_t end = + FindWebLinkEnding(str, start.value(), + TrimExternalBracketsFromWebLink( + str, start.value(), str.GetLength() - 1)); + if (end > off) { + const size_t nStart = start.value(); + const size_t nCount = end - nStart + 1; + return Link{nStart, nCount, + L"http://" + strBeCheck.Substr(nStart, nCount)}; + } } } - return false; + + return absl::nullopt; } bool CPDF_LinkExtract::CheckMailLink(WideString* str) {
diff --git a/core/fpdftext/cpdf_linkextract.h b/core/fpdftext/cpdf_linkextract.h index e86b7a7..d44db70 100644 --- a/core/fpdftext/cpdf_linkextract.h +++ b/core/fpdftext/cpdf_linkextract.h
@@ -15,6 +15,7 @@ #include "core/fxcrt/fx_coordinates.h" #include "core/fxcrt/unowned_ptr.h" #include "core/fxcrt/widestring.h" +#include "third_party/abseil-cpp/absl/types/optional.h" class CPDF_TextPage; @@ -30,16 +31,15 @@ bool GetTextRange(size_t index, int* start_char_index, int* char_count) const; protected: - bool CheckWebLink(WideString* str, int32_t* nStart, int32_t* nCount); - bool CheckMailLink(WideString* str); - - private: struct Link { - int m_Start; - int m_Count; + size_t m_Start; + size_t m_Count; WideString m_strUrl; }; + absl::optional<Link> CheckWebLink(const WideString& str); + bool CheckMailLink(WideString* str); + UnownedPtr<const CPDF_TextPage> const m_pTextPage; std::vector<Link> m_LinkArray; };
diff --git a/core/fpdftext/cpdf_linkextract_unittest.cpp b/core/fpdftext/cpdf_linkextract_unittest.cpp index 66b30f4..f75853e 100644 --- a/core/fpdftext/cpdf_linkextract_unittest.cpp +++ b/core/fpdftext/cpdf_linkextract_unittest.cpp
@@ -65,7 +65,10 @@ // Check cases that fail to extract valid web link. // The last few are legit web addresses that we don't handle now. const wchar_t* const kInvalidCases[] = { - L"", L"http", L"www.", L"https-and-www", + L"", // Blank. + L"http", // No colon. + L"www.", // Missing domain. + L"https-and-www", // Dash not colon. L"http:/abc.com", // Missing slash. L"http://((()),", // Only invalid chars in host name. L"ftp://example.com", // Ftp scheme is not supported. @@ -73,18 +76,11 @@ L"http//[example.com", // Invalid IPv6 address. L"http//[00:00:00:00:00:00", // Invalid IPv6 address. L"http//[]", // Empty IPv6 address. - // Web addresses that in correct format that we don't handle. - L"abc.example.com", // URL without scheme. + L"abc.example.com", // URL without scheme. }; - constexpr int32_t kDefaultValue = -42; for (const wchar_t* input : kInvalidCases) { - WideString text_str(input); - int32_t start_offset = kDefaultValue; - int32_t count = kDefaultValue; - EXPECT_FALSE(extractor.CheckWebLink(&text_str, &start_offset, &count)) - << input; - EXPECT_EQ(kDefaultValue, start_offset) << input; - EXPECT_EQ(kDefaultValue, count) << input; + auto maybe_link = extractor.CheckWebLink(input); + EXPECT_FALSE(maybe_link.has_value()) << input; } // Check cases that can extract valid web link. @@ -92,8 +88,8 @@ struct ValidCase { const wchar_t* const input_string; const wchar_t* const url_extracted; - const int32_t start_offset; - const int32_t count; + const size_t start_offset; + const size_t count; }; const ValidCase kValidCases[] = { {L"http://www.example.com", L"http://www.example.com", 0, @@ -172,14 +168,10 @@ {L"www.测试.net；", L"http://www.测试.net；", 0, 11}, }; for (const auto& it : kValidCases) { - const wchar_t* const input = it.input_string; - WideString text_str(input); - int32_t start_offset = kDefaultValue; - int32_t count = kDefaultValue; - EXPECT_TRUE(extractor.CheckWebLink(&text_str, &start_offset, &count)) - << input; - EXPECT_STREQ(it.url_extracted, text_str.c_str()); - EXPECT_EQ(it.start_offset, start_offset) << input; - EXPECT_EQ(it.count, count) << input; + auto maybe_link = extractor.CheckWebLink(it.input_string); + ASSERT_TRUE(maybe_link.has_value()) << it.input_string; + EXPECT_STREQ(it.url_extracted, maybe_link.value().m_strUrl.c_str()); + EXPECT_EQ(it.start_offset, maybe_link.value().m_Start) << it.input_string; + EXPECT_EQ(it.count, maybe_link.value().m_Count) << it.input_string; } }