Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12433021:
Modify content::GetFontTable so clients can control what is read. (Closed)
|
Created:
7 years, 9 months ago by bbudge Modified:
7 years, 9 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, jam, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionModify content::GetFontTable to allow clients to control what is read.
This will be used in an upcoming PPAPI Font API. This adds an offset, and changes
the semantics so that the function will only read as much data as the client
requests. Before, the function would fail if the buffer wasn't big enough.
BUG=79375
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189505
Patch Set 1 #
Total comments: 2
Patch Set 2 : Modify param order. #
Total comments: 23
Patch Set 3 : Address first round of comments. #
Total comments: 12
Patch Set 4 : Address round 2 of comments. #Patch Set 5 : Fixes #
Total comments: 8
Patch Set 6 : Address comments. #
Total comments: 1
Patch Set 7 : Clamp offsets larger than the data, don't fail. #
Total comments: 1
Messages
Total messages: 19 (0 generated)
Brett for chrome Chris for content please re-route if there are more appropriate reviewers
lgtm https://codereview.chromium.org/12433021/diff/1/content/public/common/child_p... File content/public/common/child_process_sandbox_support_linux.h (right): https://codereview.chromium.org/12433021/diff/1/content/public/common/child_p... content/public/common/child_process_sandbox_support_linux.h:42: size_t* output_length, size_t offset); I think this would make more sense after the "table" argument (also, style guide says out arguments last).
https://codereview.chromium.org/12433021/diff/1/content/public/common/child_p... File content/public/common/child_process_sandbox_support_linux.h (right): https://codereview.chromium.org/12433021/diff/1/content/public/common/child_p... content/public/common/child_process_sandbox_support_linux.h:42: size_t* output_length, size_t offset); On 2013/03/07 23:10:26, brettw wrote: > I think this would make more sense after the "table" argument (also, style guide > says out arguments last). Done.
https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:111: length -= offset; 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.
https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:111: length -= offset; 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: > 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. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:116: length = std::min(length, *output_length); 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.
Giving Chris a break, reassigning to palmer to check for security issues.
https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pe... File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right): https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pe... chrome/renderer/pepper/pepper_flash_font_file_host.cc:57: #if defined(OS_LINUX) || defined(OS_OPENBSD) What about the other BSDs? Chromium works pretty well on FreeBSD. https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pe... chrome/renderer/pepper/pepper_flash_font_file_host.cc:64: if (content::GetFontTable(fd_, table, 0 /* offset */, TOCTOU problem here? |length| is updated by this call but |contents| does not know its (potentially new and different) size? Coupled with the double cast, I find this disturbing. Maybe just allocate a temporary char array, read into it, and then copy the result into contents. https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pp... File chrome/renderer/pepper/ppb_pdf_impl.cc (right): https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pp... chrome/renderer/pepper/ppb_pdf_impl.cc:65: *output_length = static_cast<uint32_t>(temp_size); I'd use checked_numeric_cast (from base/) instead, to ensure no truncation problems. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:102: bool GetFontTable(int fd, uint32_t table, size_t offset, Perhaps this should be off_t, since it's a file offset and not an object size. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:103: uint8_t* output, size_t* output_length) { Same here. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:108: size_t length = st.st_size; 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). https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:116: length = std::min(length, *output_length); On 2013/03/14 18:21:26, bbudge1 wrote: > 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. Yes, please. :) https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:118: ssize_t n = HANDLE_EINTR(pread(fd, output, length, offset)); Yeah, note again that pread takes an off_t offset. Basically you should be using off_t throughout. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:132: num_tables = static_cast<unsigned>(num_tables_buf[0]) << 8 | NIT: So we are expecting a 16-bit number. Why not declare num_tables as uint16_t then? https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:149: table_offset = static_cast<size_t>(entry[8]) << 24 | Are there generic ReadUint32, et c. functions we could be using here?
I changed the 'offset' parameter type to off_t. The amount of duplicated code got to be too much, so I rewrote the function to share the code that does the final checking, reading, and writing the output size to the caller. https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pe... File chrome/renderer/pepper/pepper_flash_font_file_host.cc (right): https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pe... chrome/renderer/pepper/pepper_flash_font_file_host.cc:57: #if defined(OS_LINUX) || defined(OS_OPENBSD) Perhaps, but I don't know anything about Flash. If we want to enable this code for Flash on other BSDs, it is probably best done in a separate CL. On 2013/03/14 19:26:43, Chris P. wrote: > What about the other BSDs? Chromium works pretty well on FreeBSD. https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pe... chrome/renderer/pepper/pepper_flash_font_file_host.cc:64: if (content::GetFontTable(fd_, table, 0 /* offset */, I'd rather keep changes minimal for the Flash (Flapper) plugin, and just add a 0 for the new offset parameter. On 2013/03/14 19:26:43, Chris P. wrote: > TOCTOU problem here? |length| is updated by this call but |contents| does not > know its (potentially new and different) size? Coupled with the double cast, I > find this disturbing. Maybe just allocate a temporary char array, read into it, > and then copy the result into contents. https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pp... File chrome/renderer/pepper/ppb_pdf_impl.cc (right): https://codereview.chromium.org/12433021/diff/12001/chrome/renderer/pepper/pp... chrome/renderer/pepper/ppb_pdf_impl.cc:65: *output_length = static_cast<uint32_t>(temp_size); On 2013/03/14 19:26:43, Chris P. wrote: > I'd use checked_numeric_cast (from base/) instead, to ensure no truncation > problems. Done. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:102: bool GetFontTable(int fd, uint32_t table, size_t offset, On 2013/03/14 19:26:43, Chris P. wrote: > Perhaps this should be off_t, since it's a file offset and not an object size. Done. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:103: uint8_t* output, size_t* output_length) { 'output_length' is the size of the data, so always non-negative. On 2013/03/14 19:26:43, Chris P. wrote: > Same here. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:108: size_t length = st.st_size; Thanks for pointing that out. I'll use the safe numeric cast. On 2013/03/14 19:26:43, Chris P. wrote: > 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). https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:116: length = std::min(length, *output_length); On 2013/03/14 19:26:43, Chris P. wrote: > On 2013/03/14 18:21:26, bbudge1 wrote: > > 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. > > Yes, please. :) Done. I added comments to explain this and some other strange things the code is doing. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:118: ssize_t n = HANDLE_EINTR(pread(fd, output, length, offset)); On 2013/03/14 19:26:43, Chris P. wrote: > Yeah, note again that pread takes an off_t offset. Basically you should be using > off_t throughout. Done. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:132: num_tables = static_cast<unsigned>(num_tables_buf[0]) << 8 | On 2013/03/14 19:26:43, Chris P. wrote: > NIT: So we are expecting a 16-bit number. Why not declare num_tables as uint16_t > then? Done. https://codereview.chromium.org/12433021/diff/12001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:149: table_offset = static_cast<size_t>(entry[8]) << 24 | 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: > Are there generic ReadUint32, et c. functions we could be using here?
https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:131: new uint8_t[num_tables * kDirEntrySize]); 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. https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:140: if (memcmp(entry, &table, sizeof(table)) == 0) { But table isn't necessarily big-endian? Seems like you'd want to do the reinterpret_cast thing here, too. https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:142: data_offset = So, only the last table entry affects the ultimate values of data_offset and data_length? Or should these be += ? https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:151: // 'data_length' is the length of the file data. STYLE: Document these where they are declared, up above. Also, |foo| instead of 'foo'. https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:153: data_offset += offset; 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. :) https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:157: data_length -= data_offset; Potential underflow?
My Friday was already ruined. https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:131: new uint8_t[num_tables * kDirEntrySize]); On 2013/03/15 23:16:47, Chris P. wrote: > 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. Done. https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:140: if (memcmp(entry, &table, sizeof(table)) == 0) { That's true. I don't think this code was endian-correct before. Done. On 2013/03/15 23:16:47, Chris P. wrote: > But table isn't necessarily big-endian? Seems like you'd want to do the > reinterpret_cast thing here, too. https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:142: data_offset = 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: > So, only the last table entry affects the ultimate values of data_offset and > data_length? Or should these be += ? https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:151: // 'data_length' is the length of the file data. On 2013/03/15 23:16:47, Chris P. wrote: > STYLE: Document these where they are declared, up above. Also, |foo| instead of > 'foo'. Done. https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:153: data_offset += offset; Whew, that should be easier. Done. On 2013/03/15 23:16:47, Chris P. wrote: > 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. :) https://codereview.chromium.org/12433021/diff/36001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:157: data_length -= data_offset; That is checked above, with max_offset. On 2013/03/15 23:16:47, Chris P. wrote: > Potential underflow?
Tested against new font API and fixed some bugs. I also reworked the common epilogue where data gets written. PTAL
https://codereview.chromium.org/12433021/diff/43001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/43001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:106: // To prevent overflow, limit offset to 2 GB. If off_t is 64 bits (as it can be), this allows offsets > 2 GiB. So the comment doesn't quite match the code. https://codereview.chromium.org/12433021/diff/43001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:137: for (uint16_t i = 0; i < num_tables; i++) { MINOR NIT: Chromium style is to use pre-increment (++i). https://codereview.chromium.org/12433021/diff/43001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:164: data_length = std::min(data_length, *output_length); 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.) https://codereview.chromium.org/12433021/diff/43001/content/public/common/chi... File content/public/common/child_process_sandbox_support_linux.h (right): https://codereview.chromium.org/12433021/diff/43001/content/public/common/chi... content/public/common/child_process_sandbox_support_linux.h:36: // The offset must be non-negative. Update this to say "between 0 and 2 GiB - 1", or whatever is appropriate.
https://codereview.chromium.org/12433021/diff/43001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/43001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:106: // To prevent overflow, limit offset to 2 GB. On 2013/03/19 21:35:15, Chris P. wrote: > If off_t is 64 bits (as it can be), this allows offsets > 2 GiB. So the comment > doesn't quite match the code. Done. Removed the comment. https://codereview.chromium.org/12433021/diff/43001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:137: for (uint16_t i = 0; i < num_tables; i++) { On 2013/03/19 21:35:15, Chris P. wrote: > MINOR NIT: Chromium style is to use pre-increment (++i). Done. https://codereview.chromium.org/12433021/diff/43001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:164: data_length = std::min(data_length, *output_length); We write the number of bytes into their output_length parameter. On 2013/03/19 21:35:15, Chris P. wrote: > 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.) https://codereview.chromium.org/12433021/diff/43001/content/public/common/chi... File content/public/common/child_process_sandbox_support_linux.h (right): https://codereview.chromium.org/12433021/diff/43001/content/public/common/chi... content/public/common/child_process_sandbox_support_linux.h:36: // The offset must be non-negative. On 2013/03/19 21:35:15, Chris P. wrote: > Update this to say "between 0 and 2 GiB - 1", or whatever is appropriate. Done. https://codereview.chromium.org/12433021/diff/58001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/58001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:154: static const off_t kMaxPositiveOffset32 = 0x7FFFFFFF; // 2 GB - 1. I made this limit one less, as the previous one isn't quite right. Only one offset is provided by the user. The other offset from the font file shouldn't get near this.
Ready for another look.
One more tweak. https://codereview.chromium.org/12433021/diff/65001/content/common/child_proc... File content/common/child_process_sandbox_support_impl_linux.cc (right): https://codereview.chromium.org/12433021/diff/65001/content/common/child_proc... content/common/child_process_sandbox_support_impl_linux.cc:154: offset = std::min(offset, base::checked_numeric_cast<off_t>(data_length)); Without this change, it's difficult to match font table utilities on other platforms, which return 0 bytes in this case.
LGTM. Thanks for putting up with my type persnickitiness. :)
On 2013/03/20 22:14:15, Chris P. wrote: > LGTM. Thanks for putting up with my type persnickitiness. :) The code turned out much better. Thank you!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/bbudge@chromium.org/12433021/65001
Message was sent while issue was closed.
Change committed as 189505 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
