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

pdf/pdfium/pdfium_page.cc

Index: pdf/pdfium/pdfium_page.cc
diff --git a/pdf/pdfium/pdfium_page.cc b/pdf/pdfium/pdfium_page.cc
index adf9cac484bfd1334da8f69c38e491fa6760517c..0e82fe6aa7ad779b633250febba579c6a8ca89d6 100644
--- a/pdf/pdfium/pdfium_page.cc
+++ b/pdf/pdfium/pdfium_page.cc
@@ -197,14 +197,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');

[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);
@@ -323,9 +319,22 @@ 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) {
+              // 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;
@@ -407,9 +416,21 @@ void PDFiumPage::CalculateLinks() {
     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);