PDF: Yet another stab at getting WriteInto() buffer sizes correct.

- Fix remaining case with extra null-character.
- Do not assume strings are null-terminated, even though they most likely are.
- Fix accessibility code that checked for the extra null-character.

Review URL: https://codereview.chromium.org/848073003

Cr-Commit-Position: refs/heads/master@{#311715}

5 files changed, 147 insertions, 27 deletions

diff --git a/pdf/pdfium/pdfium_api_string_buffer_adapter.cc b/pdf/pdfium/pdfium_api_string_buffer_adapter.cc
new file mode 100644
index 0000000..92b46d2
--- /dev/null
+++ b/pdf/pdfium/pdfium_api_string_buffer_adapter.cc
@@ -0,0 +1,65 @@
+// Copyright 2015 The Chromium 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 "pdf/pdfium/pdfium_api_string_buffer_adapter.h"
+
+#include <string>
+
+#include "base/logging.h"
+#include "base/numerics/safe_math.h"
+#include "base/strings/string16.h"
+#include "base/strings/string_util.h"
+
+namespace chrome_pdf {
+
+template <class StringType>
+PDFiumAPIStringBufferAdapter<StringType>::PDFiumAPIStringBufferAdapter(
+    StringType* str,
+    size_t expected_size,
+    bool check_expected_size)
+    : str_(str),
+      data_(WriteInto(str, expected_size + 1)),
+      expected_size_(expected_size),
+      check_expected_size_(check_expected_size),
+      is_closed_(false) {
+}
+
+template <class StringType>
+PDFiumAPIStringBufferAdapter<StringType>::~PDFiumAPIStringBufferAdapter() {
+  DCHECK(is_closed_);
+}
+
+template <class StringType>
+void* PDFiumAPIStringBufferAdapter<StringType>::GetData() {
+  DCHECK(!is_closed_);
+  return data_;
+}
+
+template <class StringType>
+void PDFiumAPIStringBufferAdapter<StringType>::Close(int actual_size) {
+  base::CheckedNumeric<size_t> unsigned_size = actual_size;
+  Close(unsigned_size.ValueOrDie());
+}
+
+template <class StringType>
+void PDFiumAPIStringBufferAdapter<StringType>::Close(size_t actual_size) {
+  DCHECK(!is_closed_);
+  is_closed_ = true;
+
+  if (check_expected_size_)
+    DCHECK_EQ(expected_size_, actual_size);
+
+  if (actual_size > 0) {
+    DCHECK((*str_)[actual_size - 1] == 0);
+    str_->resize(actual_size - 1);
+  } else {
+    str_->clear();
+  }
+}
+
+// explicit instantiations
+template class PDFiumAPIStringBufferAdapter<std::string>;
+template class PDFiumAPIStringBufferAdapter<base::string16>;
+
+}  // namespace chrome_pdf
diff --git a/pdf/pdfium/pdfium_api_string_buffer_adapter.h b/pdf/pdfium/pdfium_api_string_buffer_adapter.h
new file mode 100644
index 0000000..a36879f
--- /dev/null
+++ b/pdf/pdfium/pdfium_api_string_buffer_adapter.h
@@ -0,0 +1,53 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef PDF_PDFIUM_PDFIUM_API_STRING_BUFFER_ADAPTER_H_
+#define PDF_PDFIUM_PDFIUM_API_STRING_BUFFER_ADAPTER_H_
+
+#include "base/basictypes.h"
+
+namespace chrome_pdf {
+
+// Helper to deal with the fact that many PDFium APIs write the null-terminator
+// into string buffers that are passed to them, but the PDF plugin likes to pass
+// in std::strings / base::string16s, where one should not count on the internal
+// string buffers to be null-terminated.
+
+template <class StringType>
+class PDFiumAPIStringBufferAdapter {
+ public:
+  // |str| is the string to write into.
+  // |expected_size| is the number of characters the PDFium API will write,
+  // including the null-terminator. It should be at least 1.
+  // |check_expected_size| whether to check the actual number of characters
+  // written into |str| against |expected_size| when calling Close().
+  PDFiumAPIStringBufferAdapter(StringType* str,
+                               size_t expected_size,
+                               bool check_expected_size);
+  ~PDFiumAPIStringBufferAdapter();
+
+  // Returns a pointer to |str_|'s buffer. The buffer's size is large enough to
+  // hold |expected_size_| + 1 characters, so the PDFium API that uses the
+  // pointer has space to write a null-terminator.
+  void* GetData();
+
+  // Resizes |str_| to |actual_size| - 1 characters, thereby removing the extra
+  // null-terminator. This must be called prior to the adapter's destruction.
+  // The pointer returned by GetData() should be considered invalid.
+  void Close(int actual_size);
+  void Close(size_t actual_size);
+
+ private:
+  StringType* const str_;
+  void* const data_;
+  const size_t expected_size_;
+  const bool check_expected_size_;
+  bool is_closed_;
+
+  DISALLOW_COPY_AND_ASSIGN(PDFiumAPIStringBufferAdapter);
+};
+
+}  // namespace chrome_pdf
+
+#endif  // PDF_PDFIUM_PDFIUM_API_STRING_BUFFER_ADAPTER_H_
diff --git a/pdf/pdfium/pdfium_engine.cc b/pdf/pdfium/pdfium_engine.cc
index 228a889..87f8d65 100644
--- a/pdf/pdfium/pdfium_engine.cc
+++ b/pdf/pdfium/pdfium_engine.cc
@@ -16,6 +16,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "pdf/draw_utils.h"
+#include "pdf/pdfium/pdfium_api_string_buffer_adapter.h"
 #include "pdf/pdfium/pdfium_mem_buffer_file_read.h"
 #include "pdf/pdfium/pdfium_mem_buffer_file_write.h"
 #include "ppapi/c/pp_errors.h"
@@ -2068,18 +2069,16 @@ void PDFiumEngine::SearchUsingICU(const base::string16& term,
   if (text_length <= 0)
     return;
 
+  PDFiumAPIStringBufferAdapter<base::string16> api_string_adapter(&page_text,
+                                                                  text_length,
+                                                                  false);
   unsigned short* data =
-      reinterpret_cast<unsigned short*>(WriteInto(&page_text, text_length + 1));
-  // |written| includes the trailing terminator, so get rid of the trailing
-  // NUL character by calling resize().
+      reinterpret_cast<unsigned short*>(api_string_adapter.GetData());
   int written = FPDFText_GetText(pages_[current_page]->GetTextPage(),
                                  character_to_start_searching_from,
                                  text_length,
                                  data);
-  if (written < 1)
-    page_text.resize(0);
-  else
-    page_text.resize(written - 1);
+  api_string_adapter.Close(written);
 
   std::vector<PDFEngine::Client::SearchStringResult> results;
   client_->SearchString(
diff --git a/pdf/pdfium/pdfium_page.cc b/pdf/pdfium/pdfium_page.cc
index adf9cac..1335f07 100644
--- a/pdf/pdfium/pdfium_page.cc
+++ b/pdf/pdfium/pdfium_page.cc
@@ -11,6 +11,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
+#include "pdf/pdfium/pdfium_api_string_buffer_adapter.h"
 #include "pdf/pdfium/pdfium_engine.h"
 
 // Used when doing hit detection.
@@ -197,14 +198,10 @@ base::Value* PDFiumPage::GetTextBoxAsValue(double page_height,
   } else if (area == WEBLINK_AREA && !link) {
     size_t start = 0;
     for (size_t i = 0; i < targets.size(); ++i) {
-      // Remove the extra NULL character at end.
-      // Otherwise, find() will not return any matches.
-      if (targets[i].url.size() > 0 &&
-          targets[i].url[targets[i].url.size() - 1] == '\0') {
-        targets[i].url.resize(targets[i].url.size() - 1);
-      }
-      // There should only ever be one NULL character
-      DCHECK(targets[i].url[targets[i].url.size() - 1] != '\0');
+      // If there is an extra NULL character at end, find() will not return any
+      // matches. There should not be any though.
+      if (!targets[i].url.empty())
+        DCHECK(targets[i].url[targets[i].url.size() - 1] != '\0');
 
       // PDFium may change the case of generated links.
       std::string lowerCaseURL = base::StringToLowerASCII(targets[i].url);
@@ -323,9 +320,13 @@ PDFiumPage::Area PDFiumPage::GetLinkTarget(
           if (target) {
             size_t buffer_size =
                 FPDFAction_GetURIPath(engine_->doc(), action, NULL, 0);
-            if (buffer_size > 1) {
-              void* data = WriteInto(&target->url, buffer_size);
-              FPDFAction_GetURIPath(engine_->doc(), action, data, buffer_size);
+            if (buffer_size > 0) {
+              PDFiumAPIStringBufferAdapter<std::string> api_string_adapter(
+                  &target->url, buffer_size, true);
+              void* data = api_string_adapter.GetData();
+              size_t bytes_written = FPDFAction_GetURIPath(
+                  engine_->doc(), action, data, buffer_size);
+              api_string_adapter.Close(bytes_written);
             }
           }
           return WEBLINK_AREA;
@@ -407,9 +408,12 @@ void PDFiumPage::CalculateLinks() {
     base::string16 url;
     int url_length = FPDFLink_GetURL(links, i, NULL, 0);
     if (url_length > 0) {
+      PDFiumAPIStringBufferAdapter<base::string16> api_string_adapter(
+          &url, url_length, true);
       unsigned short* data =
-          reinterpret_cast<unsigned short*>(WriteInto(&url, url_length + 1));
-      FPDFLink_GetURL(links, i, data, url_length);
+          reinterpret_cast<unsigned short*>(api_string_adapter.GetData());
+      int actual_length = FPDFLink_GetURL(links, i, data, url_length);
+      api_string_adapter.Close(actual_length);
     }
     Link link;
     link.url = base::UTF16ToUTF8(url);
diff --git a/pdf/pdfium/pdfium_range.cc b/pdf/pdfium/pdfium_range.cc
index 226d512..b0474fc 100644
--- a/pdf/pdfium/pdfium_range.cc
+++ b/pdf/pdfium/pdfium_range.cc
@@ -6,6 +6,7 @@
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
+#include "pdf/pdfium/pdfium_api_string_buffer_adapter.h"
 
 namespace chrome_pdf {
 
@@ -66,15 +67,13 @@ base::string16 PDFiumRange::GetText() {
   }
 
   if (count > 0) {
+    PDFiumAPIStringBufferAdapter<base::string16> api_string_adapter(&rv,
+                                                                    count,
+                                                                    false);
     unsigned short* data =
-        reinterpret_cast<unsigned short*>(WriteInto(&rv, count + 1));
-    // |written| includes the trailing terminator, so get rid of the trailing
-    // NUL character by calling resize().
+        reinterpret_cast<unsigned short*>(api_string_adapter.GetData());
     int written = FPDFText_GetText(page_->GetTextPage(), index, count, data);
-    if (written < 1)
-      rv.resize(0);
-    else
-      rv.resize(written - 1);
+    api_string_adapter.Close(written);
   }
 
   return rv;