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..43c7e4e79159c06549c0a396a2df0b4de1e2d9ac 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,79 +101,72 @@ int MatchFontWithFallback(const std::string& face, bool bold,
   return fd;
 }
 
-bool GetFontTable(int fd, uint32_t table, uint8_t* output,
-                  size_t* output_length) {
-  if (table == 0) {
+bool GetFontTable(int fd, uint32_t table_tag, off_t offset,
+                  uint8_t* output, size_t* output_length) {
+  // To prevent overflow, limit offset to 2 GB.

[palmer, 2013/03/19 21:35:15]

If off_t is 64 bits (as it can be), this allows offsets > 2 GiB. So the comment
doesn't quite match the code.

[bbudge, 2013/03/19 23:09:17]

Done. Removed the comment.

+  if (offset < 0)
+    return false;
+
+  size_t data_length = 0;  // the length of the file data.
+  off_t data_offset = 0;   // the offset of the data in the file.
+  if (table_tag == 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.
+    static const size_t kTableEntrySize = 16;
+    const size_t directory_size = num_tables * kTableEntrySize;
+    scoped_array<uint8_t> table_entries(new uint8_t[directory_size]);
+    n = HANDLE_EINTR(pread(fd, table_entries.get(), directory_size,
+                           12 /* skip the SFNT header */));
+    if (n != base::checked_numeric_cast<ssize_t>(directory_size))
       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++) {

[palmer, 2013/03/19 21:35:15]

MINOR NIT: Chromium style is to use pre-increment (++i).

[bbudge, 2013/03/19 23:09:17]

Done.

+      uint8_t* entry = table_entries.get() + i * kTableEntrySize;
+      uint32_t tag = base::NetToHost32(*reinterpret_cast<uint32_t*>(entry));
+      if (tag == table_tag) {
+        // Font data is stored in net (big-endian) order.
+        data_offset =
+            base::NetToHost32(*reinterpret_cast<uint32_t*>(entry + 8));
+        data_length =
+            base::NetToHost32(*reinterpret_cast<uint32_t*>(entry + 12));
+        break;
+      }
     }
   }
 
-  if (!length)
-    return false;
-
-  if (!output) {
-    *output_length = length;
-    return true;
-  }
-
-  if (*output_length < length)
+  if (!data_length || offset > base::checked_numeric_cast<off_t>(data_length))
     return false;
-
-  *output_length = length;
-  n = HANDLE_EINTR(pread(fd, output, length, offset));
-  if (n != static_cast<ssize_t>(length))
+  // Make sure it's safe to add the data offset and the caller's logical offset.
+  // Define the maximum positive offset on 32 bit systems.
+  static const off_t kMaxPositiveOffset32 = 1 << 30;
+  if ((offset > kMaxPositiveOffset32 / 2) ||
+      (data_offset > kMaxPositiveOffset32 / 2))
     return false;
+  data_offset += offset;
+  data_length -= offset;
+
+  if (output) {
+    // 'output_length' holds the maximum amount of data the caller can accept.
+    data_length = std::min(data_length, *output_length);

[palmer, 2013/03/19 21:35:15]

This might mean that the caller won't get a complete font table. Do we have a
way of telling them that? (Does it matter? I would assume so but don't know.)

[bbudge, 2013/03/19 23:09:17]

We write the number of bytes into their output_length parameter.
On 2013/03/19 21:35:15, Chris P. wrote:

+    ssize_t n = HANDLE_EINTR(pread(fd, output, data_length, data_offset));
+    if (n != base::checked_numeric_cast<ssize_t>(data_length))
+      return false;
+  }
+  *output_length = data_length;
 
   return true;
 }