Modify content::GetFontTable so clients can control what is read.

content/common/child_process_sandbox_support_impl_linux.cc

Index: content/common/child_process_sandbox_support_impl_linux.cc
diff --git a/content/common/child_process_sandbox_support_impl_linux.cc b/content/common/child_process_sandbox_support_impl_linux.cc
index 984f796f728bb82c76add00c74cf59aef4ec082a..3396617b54222f04c58cd8ef49a45e6ef8a3eca5 100644
--- a/content/common/child_process_sandbox_support_impl_linux.cc
+++ b/content/common/child_process_sandbox_support_impl_linux.cc
@@ -10,6 +10,8 @@
 #include "base/pickle.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/posix/unix_domain_socket_linux.h"
+#include "base/safe_numerics.h"
+#include "base/sys_byteorder.h"
 #include "content/common/sandbox_linux.h"
 #include "third_party/WebKit/Source/Platform/chromium/public/linux/WebFontFamily.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/linux/WebFontRenderStyle.h"
@@ -99,78 +101,71 @@ int MatchFontWithFallback(const std::string& face, bool bold,
   return fd;
 }
 
-bool GetFontTable(int fd, uint32_t table, uint8_t* output,
-                  size_t* output_length) {
+bool GetFontTable(int fd, uint32_t table, off_t offset,
+                  uint8_t* output, size_t* output_length) {
+  if (offset < 0)
+    return false;
+
+  size_t data_length = 0;
+  off_t data_offset = 0;
   if (table == 0) {
+    // Get the entire font file.
     struct stat st;
     if (fstat(fd, &st) < 0)
       return false;
-    size_t length = st.st_size;
-    if (!output) {
-      *output_length = length;
-      return true;
-    }
-    if (*output_length < length)
+    data_length = base::checked_numeric_cast<size_t>(st.st_size);
+  } else {
+    // Get a font table. Read the header to find its offset in the file.
+    uint16_t num_tables;
+    ssize_t n = HANDLE_EINTR(pread(fd, &num_tables, sizeof(num_tables),
+                             4 /* skip the font type */));
+    if (n != sizeof(num_tables))
       return false;
-    *output_length = length;
-    ssize_t n = HANDLE_EINTR(pread(fd, output, length, 0));
-    if (n != static_cast<ssize_t>(length))
+    // Font data is stored in net (big-endian) order.
+    num_tables = base::NetToHost16(num_tables);
+
+    // Read the table directory.
+    // The size in bytes of an entry in the table directory.
+    static const size_t kDirEntrySize = 16;
+    scoped_array<uint8_t> table_entries(
+        new uint8_t[num_tables * kDirEntrySize]);

[palmer, 2013/03/15 23:16:47]

OPTIONAL NIT: Maybe give this expression a name to ensure consistency in future
updates to the code.

    const size_t table_entries_size = num_tables * kDirEntrySize;

Then use that name throughout the function body.

[bbudge, 2013/03/16 00:07:03]

Done.

+    n = HANDLE_EINTR(pread(fd, table_entries.get(), num_tables * kDirEntrySize,
+                           12 /* skip the SFNT header */));
+    if (n != base::checked_numeric_cast<ssize_t>(num_tables * kDirEntrySize))
       return false;
-    return true;
-  }
-
-  unsigned num_tables;
-  uint8_t num_tables_buf[2];
 
-  ssize_t n = HANDLE_EINTR(pread(fd, &num_tables_buf, sizeof(num_tables_buf),
-                           4 /* skip the font type */));
-  if (n != sizeof(num_tables_buf))
-    return false;
-
-  num_tables = static_cast<unsigned>(num_tables_buf[0]) << 8 |
-               num_tables_buf[1];
-
-  // The size in bytes of an entry in the table directory.
-  static const unsigned kTableEntrySize = 16;
-  scoped_array<uint8_t> table_entries(
-      new uint8_t[num_tables * kTableEntrySize]);
-  n = HANDLE_EINTR(pread(fd, table_entries.get(), num_tables * kTableEntrySize,
-                         12 /* skip the SFNT header */));
-  if (n != static_cast<ssize_t>(num_tables * kTableEntrySize))
-    return false;
-
-  size_t offset;
-  size_t length = 0;
-  for (unsigned i = 0; i < num_tables; i++) {
-    const uint8_t* entry = table_entries.get() + i * kTableEntrySize;
-    if (memcmp(entry, &table, sizeof(table)) == 0) {
-      offset = static_cast<size_t>(entry[8]) << 24 |
-               static_cast<size_t>(entry[9]) << 16 |
-               static_cast<size_t>(entry[10]) << 8  |
-               static_cast<size_t>(entry[11]);
-      length = static_cast<size_t>(entry[12]) << 24 |
-               static_cast<size_t>(entry[13]) << 16 |
-               static_cast<size_t>(entry[14]) << 8  |
-               static_cast<size_t>(entry[15]);
-
-      break;
+    for (uint16_t i = 0; i < num_tables; i++) {
+      uint8_t* entry = table_entries.get() + i * kDirEntrySize;
+      // The tag is stored in big-endian order at the start of the entry.
+      if (memcmp(entry, &table, sizeof(table)) == 0) {

[palmer, 2013/03/15 23:16:47]

But table isn't necessarily big-endian? Seems like you'd want to do the
reinterpret_cast thing here, too.

[bbudge, 2013/03/16 00:07:03]

That's true. I don't think this code was endian-correct before. Done.
On 2013/03/15 23:16:47, Chris P. wrote:

+        // Font data is stored in net (big-endian) order.
+        data_offset =

[palmer, 2013/03/15 23:16:47]

So, only the last table entry affects the ultimate values of data_offset and
data_length? Or should these be += ?

[bbudge, 2013/03/16 00:07:03]

Only the entry with a matching tag. Offsets are absolute, from the beginning of
the file.
On 2013/03/15 23:16:47, Chris P. wrote:

+            base::NetToHost32(*reinterpret_cast<uint32_t*>(entry + 8));
+        data_length =
+            base::NetToHost32(*reinterpret_cast<uint32_t*>(entry + 12));
+        break;
+      }
     }
   }
 
-  if (!length)
+  // 'data_length' is the length of the file data.

[palmer, 2013/03/15 23:16:47]

STYLE: Document these where they are declared, up above. Also, |foo| instead of
'foo'.

[bbudge, 2013/03/16 00:07:03]

Done.

+  // 'data_offset' is its offset in the file.
+  data_offset += offset;

[palmer, 2013/03/15 23:16:47]

offset + user_controlled data could overflow, of off_t is 32 bits on the
platform or if offset as passed was particularly large (highly unlikely; just
saying).

Worse, off_t is signed, so overflow behavior is undefined. :] (I'd resolve this
by first checking that offset is too small to make data_offset overflow, and
then doing the addition.)

Parsing binary data is hard; I'm not trying to ruin your Friday I promise. :)

[bbudge, 2013/03/16 00:07:03]

Whew, that should be easier. Done.
On 2013/03/15 23:16:47, Chris P. wrote:

+  off_t max_offset = base::checked_numeric_cast<off_t>(data_length);
+  if (!data_length || data_offset > max_offset)
     return false;
+  data_length -= data_offset;

[palmer, 2013/03/15 23:16:47]

Potential underflow?

[bbudge, 2013/03/16 00:07:03]

That is checked above, with max_offset.
On 2013/03/15 23:16:47, Chris P. wrote:

 
   if (!output) {
-    *output_length = length;
+    *output_length = data_length;
     return true;
   }
 
-  if (*output_length < length)
-    return false;
-
-  *output_length = length;
-  n = HANDLE_EINTR(pread(fd, output, length, offset));
-  if (n != static_cast<ssize_t>(length))
+  // 'output_length' holds the maximum amount of data the caller can accept.
+  data_length = std::min(data_length, *output_length);
+  *output_length = data_length;
+  ssize_t n = HANDLE_EINTR(pread(fd, output, data_length, data_offset));
+  if (n != base::checked_numeric_cast<ssize_t>(data_length))
     return false;
 
   return true;