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..b5e341ded7e15df0dd1f01181c18c67f4f13e228 100644
--- a/content/common/child_process_sandbox_support_impl_linux.cc
+++ b/content/common/child_process_sandbox_support_impl_linux.cc
@@ -99,21 +99,23 @@ 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, size_t offset,

[palmer, 2013/03/14 19:26:43]

Perhaps this should be off_t, since it's a file offset and not an object size.

[bbudge, 2013/03/15 22:25:34]

Done.

+                  uint8_t* output, size_t* output_length) {

[palmer, 2013/03/14 19:26:43]

Same here.

[bbudge, 2013/03/15 22:25:34]

'output_length' is the size of the data, so always non-negative.
On 2013/03/14 19:26:43, Chris P. wrote:

   if (table == 0) {
     struct stat st;
     if (fstat(fd, &st) < 0)
       return false;
     size_t length = st.st_size;

[palmer, 2013/03/14 19:26:43]

st_size is an off_t, which is not necessarily the same as a size_t. You should
resolve this confusion somehow (use off_t, or use checked_numeric_cast, or the
like).

[bbudge, 2013/03/15 22:25:34]

Thanks for pointing that out. I'll use the safe numeric cast.
On 2013/03/14 19:26:43, Chris P. wrote:

+    if (offset > length)
+      return false;
+    length -= offset;

[Chris Evans, 2013/03/14 18:15:30]

I don't understand this change. This could be because I'm a bit dazed today. You
may wish to find a better reviewer ;-)

Anyway, it still looks like it'll load most of the file, i.e. the entirety of
the file from the offset up until the end? That sounds contrary to the stated
intend of the CL.

[bbudge, 2013/03/14 18:21:26]

It's a little tricky but the output_length parameter is now an in/out param.
It's used below to limit what's read. This code is just sanity checking that
offset isn't off the end of the file.

On 2013/03/14 18:15:30, Chris Evans wrote:

     if (!output) {
       *output_length = length;
       return true;
     }
-    if (*output_length < length)
-      return false;
+    length = std::min(length, *output_length);

[bbudge, 2013/03/14 18:21:26]

This is where the caller's output_len can limit the length read. If you'd like,
I can add comments to explain what's happening.

[palmer, 2013/03/14 19:26:43]

Yes, please. :)

[bbudge, 2013/03/15 22:25:34]

Done. I added comments to explain this and some other strange things the code is
doing.

     *output_length = length;
-    ssize_t n = HANDLE_EINTR(pread(fd, output, length, 0));
+    ssize_t n = HANDLE_EINTR(pread(fd, output, length, offset));

[palmer, 2013/03/14 19:26:43]

Yeah, note again that pread takes an off_t offset. Basically you should be using
off_t throughout.

[bbudge, 2013/03/15 22:25:34]

Done.

     if (n != static_cast<ssize_t>(length))
       return false;
     return true;
@@ -139,15 +141,15 @@ bool GetFontTable(int fd, uint32_t table, uint8_t* output,
   if (n != static_cast<ssize_t>(num_tables * kTableEntrySize))
     return false;
 
-  size_t offset;
+  size_t table_offset = 0;
   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]);
+      table_offset = static_cast<size_t>(entry[8]) << 24 |

[palmer, 2013/03/14 19:26:43]

Are there generic ReadUint32, et c. functions we could be using here?

[bbudge, 2013/03/15 22:25:34]

There are the base::NetToHost functions. The file is always stored in big-endian
order, so those will work. Done.
On 2013/03/14 19:26:43, Chris P. wrote:

+                     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  |
@@ -157,19 +159,18 @@ bool GetFontTable(int fd, uint32_t table, uint8_t* output,
     }
   }
 
-  if (!length)
+  if (!length || offset > length)
     return false;
+  length -= offset;
 
   if (!output) {
     *output_length = length;
     return true;
   }
 
-  if (*output_length < length)
-    return false;
-
+  length = std::min(length, *output_length);
   *output_length = length;
-  n = HANDLE_EINTR(pread(fd, output, length, offset));
+  n = HANDLE_EINTR(pread(fd, output, length, table_offset + offset));
   if (n != static_cast<ssize_t>(length))
     return false;