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

content/common/child_process_sandbox_support_impl_linux.cc

 // Copyright (c) 2012 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 "content/common/child_process_sandbox_support_impl_linux.h"
 
 #include <sys/stat.h>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/pickle.h"
@@ skipping 81 matching lines @@
   request.WriteBool(bold);
   request.WriteBool(italic);
   request.WriteUInt32(charset);
   uint8_t reply_buf[64];
   int fd = -1;
   UnixDomainSocket::SendRecvMsg(GetSandboxFD(), reply_buf, sizeof(reply_buf),
                                 &fd, request);
   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;
   }
 
   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 |

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

NIT: So we are expecting a 16-bit number. Why not declare num_tables as uint16_t
then?

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

Done.

                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 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  |
                static_cast<size_t>(entry[15]);
 
       break;
     }
   }
 
-  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;
 
   return true;
 }
 
 }  // namespace content