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

pdf/pdfium/pdfium_page.cc

 // Copyright (c) 2010 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_page.h"
 
 #include <math.h>
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 179 matching lines @@
   base::ListValue* text_nodes = new base::ListValue();
 
   if (area == DOCLINK_AREA) {
     std::string url = kDocLinkURLPrefix + base::IntToString(targets[0].page);
     text_nodes->Append(CreateURLNode(text_utf8, url));
   } else if (area == WEBLINK_AREA && link) {
     text_nodes->Append(CreateURLNode(text_utf8, targets[0].url));
   } 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');

[raymes, 2015/01/14 05:05:52]

just double checking: does DCHECK ever evaluate to an empty string?

[Lei Zhang, 2015/01/15 00:42:08]

Sorry, having a bit of trouble parsing the question. We don't go out of bounds
due to the empty() check above if that's what you are asking.

[raymes, 2015/01/15 01:22:57]

I was just checking that it's ok to omit the {} from the if-statement in this
case where the DCHECK macro is. I think it is.

 
       // PDFium may change the case of generated links.
       std::string lowerCaseURL = base::StringToLowerASCII(targets[i].url);
       std::string lowerCaseText = base::StringToLowerASCII(text_utf8);
       size_t pos = lowerCaseText.find(lowerCaseURL, start);
       size_t length = targets[i].url.size();
       if (pos == std::string::npos) {
         // Check if the link is a "mailto:" URL
         if (lowerCaseURL.compare(0, 7, "mailto:") == 0) {
           pos = lowerCaseText.find(lowerCaseURL.substr(7), start);
@@ skipping 98 matching lines @@
           if (dest)
             return GetDestinationTarget(dest, target);
           // TODO(gene): We don't fully support all types of the in-document
           // links. Need to implement that. There is a bug to track that:
           // http://code.google.com/p/chromium/issues/detail?id=55776
         } break;
       case PDFACTION_URI: {
           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) {
+              // FPDFAction_GetURIPath() will write out a null-terminated byte
+              // stream into the std::string holding |data|. Although most
+              // std::string implementations are null-terminated, the code here
+              // should not depend on this implementation detail. Thus
+              // WriteInto() allocates an extra byte to take the null-terminator
+              // and then resizes the string to get rid of it.
+              void* data = WriteInto(&target->url, buffer_size + 1);

[raymes, 2015/01/14 05:05:52]

:( you're right we need to be careful that the API call doesn't write the NULL
terminator into unmanaged memory.

We could have some scoped thingy which wraps WriteInto and does the resize on
destruction. WDYT?

[Lei Zhang, 2015/01/15 00:42:08]

I'm not sure where to put it. It could also just be a TrimNullTerminator()
function.

Also, for FPDFText_GetText(), there's no DCHECK against the expected size, but
here we have an expected size.

[raymes, 2015/01/15 01:22:57]

I guess I was thinking something like what's below. But it might be overkill to
do this. I'll leave it up to you - I'm ok either way.

APIStringWriter writer(buffer_size); // buffer size includes null bytes that may
get written into.
void* data = writer.data();
...
// GetString optionally takes the size to resize to before returning.
std::string* str = writer.GetString();

+              size_t bytes_written = FPDFAction_GetURIPath(
+                  engine_->doc(), action, data, buffer_size);

[raymes, 2015/01/14 05:05:52]

In this case can't we just DCHECK that buffer_size == bytes_written

[Lei Zhang, 2015/01/15 00:42:08]

Sure. I suppose the if condition should always be true.

+              if (bytes_written > 0) {
+                DCHECK_EQ('\0', target->url[bytes_written - 1]);
+                target->url.resize(bytes_written - 1);
+              } else {
+                target->url.clear();
+              }
             }
           }
           return WEBLINK_AREA;
         } break;
       // TODO(gene): We don't support PDFACTION_REMOTEGOTO and PDFACTION_LAUNCH
       // at the moment.
     }
   }
 
   return NONSELECTABLE_AREA;
@@ skipping 61 matching lines @@
   if (calculated_links_)
     return;
 
   calculated_links_ = true;
   FPDF_PAGELINK links = FPDFLink_LoadWebLinks(GetTextPage());
   int count = FPDFLink_CountWebLinks(links);
   for (int i = 0; i < count; ++i) {
     base::string16 url;
     int url_length = FPDFLink_GetURL(links, i, NULL, 0);
     if (url_length > 0) {
+      // FPDFLink_GetURL() will write out a null-terminated 16-bit char
+      // stream into the base::string16 holding |data|. Although string
+      // implementations may be null-terminated, the code here should not
+      // depend on this implementation detail. Thus WriteInto() allocates an
+      // extra byte to take the null-terminator and then resizes the string to
+      // get rid of it.
       unsigned short* data =
           reinterpret_cast<unsigned short*>(WriteInto(&url, url_length + 1));
-      FPDFLink_GetURL(links, i, data, url_length);
+      int actual_length = FPDFLink_GetURL(links, i, data, url_length);

[raymes, 2015/01/14 05:05:52]

in this case again I think it may be sufficient to DCHECK that this equals the
length the API returned earlier.

+      if (actual_length > 0) {
+        DCHECK_EQ(L'\0', url[actual_length - 1]);
+        url.resize(actual_length - 1);
+      } else {
+        url.clear();
+      }
     }
     Link link;
     link.url = base::UTF16ToUTF8(url);
 
     // If the link cannot be converted to a pp::Var, then it is not possible to
     // pass it to JS. In this case, ignore the link like other PDF viewers.
     // See http://crbug.com/312882 for an example.
     pp::Var link_var(link.url);
     if (!link_var.is_string())
       continue;
@@ skipping 73 matching lines @@
   page_->loading_count_--;
 }
 
 PDFiumPage::Link::Link() {
 }
 
 PDFiumPage::Link::~Link() {
 }
 
 }  // namespace chrome_pdf