Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
13918002:
[OTS] Integrate WOFF 2.0 algorithm into OTS (Closed)
|
Created:
7 years, 8 months ago by Kunihiko Sakamoto Modified:
7 years, 7 months ago Reviewers:
Yusuke Sato CC:
chromium-reviews, raph, agl Base URL:
http://ots.googlecode.com/svn/trunk/ Visibility:
Public. |
Description[OTS] Integrate WOFF 2.0 algorithm into OTS
Add woff2.h and woff2.cc from the WOFF 2.0 reference implementation
(https://code.google.com/p/font-compression-reference/).
WOFF2 support is disabled by default and is enabled by ots::EnableWOFF2().
Patch originally by bashi@chromium.org.
BUG=122603
R=yusukes@chromium.org
Committed: 100
Patch Set 1 #
Total comments: 90
Patch Set 2 : #
Total comments: 2
Patch Set 3 : #Patch Set 4 : #
Messages
Total messages: 14 (0 generated)
Yusuke-san, could you take a look?
qq: Do we really want to maintain two sets of woff2.cc/h files (one in OTS, another in font-compression-reference)? Can't we pull font-compression-reference to Chromium via DEPS and add a dependency on the library to ots.gyp (just like lzma_sdk and zlib)? I'm not familiar with the font-compression-reference project, so please let me know if you don't think it's a good idea to pull the project via Chrome DEPS.
[+Raph] Looking at https://code.google.com/p/font-compression-reference/source/browse/cpp, it looks like the cpp part of font-compression-reference is a kind of extension to OTS rather than an independent project. So perhaps we should remove cpp/ from font-compression-reference after land of this patch, and maintain them in OTS project. Raph, what do you think?
Friendly ping for raph :)
Absolutely, I'm happy for the decompression code to live in OTS as the authoritative source. I think it made more sense to have both in a separate project when the wire format was still evolving, and changes required modification to both compression and decompression concurrently. Thanks, and apologies for not responding earlier. Raph On Apr 11, 2013 8:52 PM, <ksakamoto@chromium.org> wrote: > Friendly ping for raph :) > > https://codereview.chromium.**org/13918002/<https://codereview.chromium.org/1... >
ksakamoto: I guess src/woff2.* have already been reviewed by someone else since they're already in the font-compression-reference repo. Is this correct? If so, please let me know the review URL. Also let me know if you modified woff2.* when you copied them to ots. -Yusuke On 2013/04/12 06:30:58, raph_google.com wrote: > Absolutely, I'm happy for the decompression code to live in OTS as the > authoritative source. I think it made more sense to have both in a separate > project when the wire format was still evolving, and changes required > modification to both compression and decompression concurrently. > > Thanks, and apologies for not responding earlier. > > Raph > On Apr 11, 2013 8:52 PM, <mailto:ksakamoto@chromium.org> wrote: > > > Friendly ping for raph :) > > > > > https://codereview.chromium.**org/13918002/%3Chttps://codereview.chromium.org...> > >
[+agl]
In my understanding, woff2.{h,cc} were initially submitted to
font-compression-reference without review, but afterwards reviewed by Adam
(cc'd). Review URLs:
https://codereview.appspot.com/6139064
https://codereview.appspot.com/6160043
https://codereview.appspot.com/8346043
Also, we are going to have security review before enabling woff2 in Chrome.
I applied following changes to woff2.{h,cc} when copied.
diff -u font-compression-reference/cpp/woff2.cc ots-svn/src/woff2.cc
--- font-compression-reference/cpp/woff2.cc 2013-04-08 14:59:21.053948917 +0900
+++ ots-svn/src/woff2.cc 2013-04-09 12:48:24.848338446 +0900
@@ -1,25 +1,16 @@
-// Copyright (c) 2012 Google Inc. All rights reserved.
+// Copyright (c) 2013 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.
// This is the implementation of decompression of the proposed WOFF Ultra
// Condensed file format.
-// For now, use of LZMA is conditional, because the build is trickier. When
-// that gets all sorted out, we can get rid of these ifdefs.
-#define USE_LZMA
-
#include <vector>
-
-#ifdef USE_LZMA
-#include "third_party/lzma_sdk/LzmaLib.h"
-#endif
-
#include <zlib.h>
+#include "third_party/lzma_sdk/LzmaLib.h"
#include "opentype-sanitiser.h"
#include "ots-memory-stream.h"
-
#include "ots.h"
#include "woff2.h"
@@ -727,7 +718,6 @@
return OTS_FAILURE();
}
return true;
-#ifdef USE_LZMA
} else if (compression_type == kCompressionTypeLzma) {
if (src_size < kLzmaHeaderSize) {
// Make sure we have at least a full Lzma header
@@ -743,27 +733,11 @@
return OTS_FAILURE();
}
return true;
-#endif
}
// Unknown compression type
return OTS_FAILURE();
}
-bool ReadLongDirectory(ots::Buffer* file, std::vector<Table>* tables,
- size_t num_tables) {
- for (size_t i = 0; i < num_tables; ++i) {
- Table* table = &(*tables)[i];
- if (!file->ReadU32(&table->tag) ||
- !file->ReadU32(&table->flags) ||
- !file->ReadU32(&table->src_length) ||
- !file->ReadU32(&table->transform_length) ||
- !file->ReadU32(&table->dst_length)) {
- return OTS_FAILURE();
- }
- }
- return true;
-}
-
const uint32_t known_tags[29] = {
TAG('c', 'm', 'a', 'p'), // 0
TAG('h', 'e', 'a', 'd'), // 1
Only in font-compression-reference/cpp/: woff2-decompress.cc
Only in font-compression-reference/cpp/: woff2.gyp
diff -u font-compression-reference/cpp/woff2.h ots-svn/src/woff2.h
--- font-compression-reference/cpp/woff2.h 2013-04-08 14:31:21.860168235 +0900
+++ ots-svn/src/woff2.h 2013-04-09 12:48:24.848338446 +0900
@@ -1,4 +1,4 @@
-// Copyright (c) 2012 Google Inc. All rights reserved.
+// Copyright (c) 2013 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.
@@ -14,7 +14,7 @@
// be the same as determined by ComputeFinalSize(). Returns true on successful
// decompression.
bool ConvertWOFF2ToTTF(uint8_t *result, size_t result_length,
- const uint8_t *data, size_t length);
+ const uint8_t *data, size_t length);
}
initial review. sorry for the delay! https://codereview.chromium.org/13918002/diff/1/src/ots.cc File src/ots.cc (right): https://codereview.chromium.org/13918002/diff/1/src/ots.cc#newcode415 src/ots.cc:415: std::vector<uint8_t> decompressed_buffer(decompressed_size); * Do we have to zero-fill the buffer? * Please define the max decompressed size and return with OTS_FAILURE() when |decompressed_size| exceeds that. With the current code, I guess drive-by web can easily crash the renderer by oom. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc File src/woff2.cc (right): https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode8 src/woff2.cc:8: #include <vector> space between 8&9 https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode9 src/woff2.cc:9: #include <zlib.h> space between 9&10 https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode88 src/woff2.cc:88: // TODO: copied from ots.cc, probably shouldn't be duplicated. I guess we can fix this now. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode143 src/woff2.cc:143: if (result & 0xe0000000) { nit: 0xe0000000U is slightly easier to read. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode157 src/woff2.cc:157: dst[offset] = x >> 24; is there any reason not to check the buffer length on each write? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode165 src/woff2.cc:165: dst[offset] = x >> 8; the same. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode171 src/woff2.cc:171: // Precondition: 0 <= baseval < 65536 (to avoid integer overflow) Use assert() then? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode181 src/woff2.cc:181: if (n_points > in_size) { Is this correct? I guess this might be >=. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode260 src/woff2.cc:260: for (unsigned int i = 0; i < points.size(); ++i) { size_t https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode320 src/woff2.cc:320: for (unsigned int i = 0; i < points.size(); ++i) { size_t https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode325 src/woff2.cc:325: dst[x_offset++] = std::abs(dx); nit: #include <cstdlib> https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode353 src/woff2.cc:353: for (unsigned int i = 0; i < points.size(); ++i) { size_t https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode449 src/woff2.cc:449: if (offset_size * loca_size > dst_size) { please add a comment about an integer overflow (which never happens.) https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode506 src/woff2.cc:506: std::vector<uint32_t> loca_values(num_glyphs + 1); Is zero-fill required? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode530 src/woff2.cc:530: if (instruction_size + 2 > glyf_dst_size - glyph_size) { integer overflow not checked? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode596 src/woff2.cc:596: instruction_size + 2) { integer overflow not checked. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode637 src/woff2.cc:637: if (tables[i].tag == tag) { qq: is this endian neutral? (not reading the code in detail, sorry. just asking) https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode653 src/woff2.cc:653: if (static_cast<uint64_t>(glyf_table->dst_offset + glyf_table->dst_length) > what is the case for? Looks no-op to me. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode657 src/woff2.cc:657: if (static_cast<uint64_t>(loca_table->dst_offset + loca_table->dst_length) > the same. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode680 src/woff2.cc:680: checksum += (buf[i] << 24) | (buf[i + 1] << 16) | endian? (same as above. just asking) https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode692 src/woff2.cc:692: size_t adjustment_offset = head_table->dst_offset + kCheckSumAdjustmentOffset; not sure, but maybe change the type to uint64_t and check integer overflow? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode702 src/woff2.cc:702: file_checksum += checksum; integer overflow is explicitly allowed for |file_checksum|, right? please add a comment. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode741 src/woff2.cc:741: const uint32_t known_tags[29] = { kKnownTags. Move this to around line 69. I guess you can get rid of '29'. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode788 src/woff2.cc:788: if ((flag_byte & 0x1f) >= (sizeof(known_tags) / sizeof(known_tags[0]))) { optional: time to add the arraysize template function to ots? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode826 src/woff2.cc:826: table->src_length = src_length; To be on safer side, can we reject huge src_length, transform_length, and dst_length here? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode828 src/woff2.cc:828: table->dst_length = dst_length; could you assign zero to src_offset and dst_offset here just in case? uninitialized values look scary to me. or, you can simply add a ctor to the struct. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode839 src/woff2.cc:839: uint32_t total_length; IIRC, always adding '= 0;' would be better to avoid gcc warnings. please fix this and elsewhere. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode853 src/woff2.cc:853: uint32_t signature; =0 https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode854 src/woff2.cc:854: uint32_t flavor; =0 https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode855 src/woff2.cc:855: if (!file.ReadU32(&signature) || signature != kWoff2Signature || is this endian neutral? if not, better not to assume little endian? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode860 src/woff2.cc:860: // TODO(bashi): Should call IsValidVersionTag() here. File a bug? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode880 src/woff2.cc:880: // Note: change below to ReadLongDirectory to enable long format. what's "long format"? Don't we have to support it? Please add a comment for a newbie like me. Please file a bug if needed. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode889 src/woff2.cc:889: Table* table = &tables[i]; replacing all operator[] calls with at() which checks the size of the vector might be a good idea since this is a security module. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode895 src/woff2.cc:895: src_offset = Round4(src_offset); // TODO: reconsider more descriptive comment, please. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode986 src/woff2.cc:986: if (static_cast<uint64_t>(table->dst_offset + transform_length) > no-op?
Thank you for the thorough review! And sorry for the slow response. https://codereview.chromium.org/13918002/diff/1/src/ots.cc File src/ots.cc (right): https://codereview.chromium.org/13918002/diff/1/src/ots.cc#newcode415 src/ots.cc:415: std::vector<uint8_t> decompressed_buffer(decompressed_size); On 2013/04/17 18:38:46, Yusuke Sato wrote: > * Do we have to zero-fill the buffer? Done. > * Please define the max decompressed size and return with OTS_FAILURE() when > |decompressed_size| exceeds that. With the current code, I guess drive-by web > can easily crash the renderer by oom. Good catch, thanks. I set the limit to 30MB, that is somewhat stricter than other formats where sum of decompressed tables must be <=30MB. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc File src/woff2.cc (right): https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode8 src/woff2.cc:8: #include <vector> On 2013/04/17 18:38:46, Yusuke Sato wrote: > space between 8&9 Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode9 src/woff2.cc:9: #include <zlib.h> On 2013/04/17 18:38:46, Yusuke Sato wrote: > space between 9&10 Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode88 src/woff2.cc:88: // TODO: copied from ots.cc, probably shouldn't be duplicated. On 2013/04/17 18:38:46, Yusuke Sato wrote: > I guess we can fix this now. Moved to ots.h. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode143 src/woff2.cc:143: if (result & 0xe0000000) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > nit: 0xe0000000U is slightly easier to read. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode157 src/woff2.cc:157: dst[offset] = x >> 24; On 2013/04/17 18:38:46, Yusuke Sato wrote: > is there any reason not to check the buffer length on each write? I don't think so. Probably I should create 'writer' version of the Buffer class and use it across the code? (It requires a considerable amount of change though.) https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode171 src/woff2.cc:171: // Precondition: 0 <= baseval < 65536 (to avoid integer overflow) On 2013/04/17 18:38:46, Yusuke Sato wrote: > Use assert() then? Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode181 src/woff2.cc:181: if (n_points > in_size) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > Is this correct? I guess this might be >=. Correct, I think. Note that each point consumes 1-4 bytes of |in| buffer, so this is an early return with lower-bound check. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode260 src/woff2.cc:260: for (unsigned int i = 0; i < points.size(); ++i) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > size_t Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode320 src/woff2.cc:320: for (unsigned int i = 0; i < points.size(); ++i) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > size_t Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode325 src/woff2.cc:325: dst[x_offset++] = std::abs(dx); On 2013/04/17 18:38:46, Yusuke Sato wrote: > nit: #include <cstdlib> Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode353 src/woff2.cc:353: for (unsigned int i = 0; i < points.size(); ++i) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > size_t Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode449 src/woff2.cc:449: if (offset_size * loca_size > dst_size) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > please add a comment about an integer overflow (which never happens.) Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode506 src/woff2.cc:506: std::vector<uint32_t> loca_values(num_glyphs + 1); On 2013/04/17 18:38:46, Yusuke Sato wrote: > Is zero-fill required? No - all element should be written. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode530 src/woff2.cc:530: if (instruction_size + 2 > glyf_dst_size - glyph_size) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > integer overflow not checked? |instruction_size| is set by Read255UShort, so 0 <= instruction_size < 65536. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode596 src/woff2.cc:596: instruction_size + 2) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > integer overflow not checked. Same as above. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode637 src/woff2.cc:637: if (tables[i].tag == tag) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > qq: is this endian neutral? (not reading the code in detail, sorry. just asking) Yes. Tag values are either read by ReadU32 or created by TAG(). https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode653 src/woff2.cc:653: if (static_cast<uint64_t>(glyf_table->dst_offset + glyf_table->dst_length) > On 2013/04/17 18:38:46, Yusuke Sato wrote: > what is the case for? Looks no-op to me. You mean the static_cast is no-op? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode680 src/woff2.cc:680: checksum += (buf[i] << 24) | (buf[i + 1] << 16) | On 2013/04/17 18:38:46, Yusuke Sato wrote: > endian? (same as above. just asking) Looks OK to me. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode692 src/woff2.cc:692: size_t adjustment_offset = head_table->dst_offset + kCheckSumAdjustmentOffset; On 2013/04/17 18:38:46, Yusuke Sato wrote: > not sure, but maybe change the type to uint64_t and check integer overflow? Overflow check added. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode702 src/woff2.cc:702: file_checksum += checksum; On 2013/04/17 18:38:46, Yusuke Sato wrote: > integer overflow is explicitly allowed for |file_checksum|, right? please add a > comment. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode741 src/woff2.cc:741: const uint32_t known_tags[29] = { On 2013/04/17 18:38:46, Yusuke Sato wrote: > kKnownTags. Move this to around line 69. I guess you can get rid of '29'. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode788 src/woff2.cc:788: if ((flag_byte & 0x1f) >= (sizeof(known_tags) / sizeof(known_tags[0]))) { On 2013/04/17 18:38:46, Yusuke Sato wrote: > optional: time to add the arraysize template function to ots? Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode826 src/woff2.cc:826: table->src_length = src_length; On 2013/04/17 18:38:46, Yusuke Sato wrote: > To be on safer side, can we reject huge src_length, transform_length, and > dst_length here? Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode828 src/woff2.cc:828: table->dst_length = dst_length; On 2013/04/17 18:38:46, Yusuke Sato wrote: > could you assign zero to src_offset and dst_offset here just in case? > uninitialized values look scary to me. > > or, you can simply add a ctor to the struct. Constructor added. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode839 src/woff2.cc:839: uint32_t total_length; On 2013/04/17 18:38:46, Yusuke Sato wrote: > IIRC, always adding '= 0;' would be better to avoid gcc warnings. please fix > this and elsewhere. Hmm gcc -Wall does not generate any warnings on this? https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode855 src/woff2.cc:855: if (!file.ReadU32(&signature) || signature != kWoff2Signature || On 2013/04/17 18:38:46, Yusuke Sato wrote: > is this endian neutral? if not, better not to assume little endian? ReadU32 reads big endian so this is endian neutral. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode860 src/woff2.cc:860: // TODO(bashi): Should call IsValidVersionTag() here. On 2013/04/17 18:38:46, Yusuke Sato wrote: > File a bug? Now it's easy, as this is a change to ots repository. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode880 src/woff2.cc:880: // Note: change below to ReadLongDirectory to enable long format. On 2013/04/17 18:38:46, Yusuke Sato wrote: > what's "long format"? Don't we have to support it? Please add a comment for a > newbie like me. Please file a bug if needed. I guess we don't. Checking with David. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode889 src/woff2.cc:889: Table* table = &tables[i]; On 2013/04/17 18:38:46, Yusuke Sato wrote: > replacing all operator[] calls with at() which checks the size of the vector > might be a good idea since this is a security module. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode895 src/woff2.cc:895: src_offset = Round4(src_offset); // TODO: reconsider On 2013/04/17 18:38:46, Yusuke Sato wrote: > more descriptive comment, please. I think this comment is obsolete. Removed.
Yusuke-san, could you take another look? Thanks!
LGTM Please ask the security team to review the code before enabling this on Chrome. https://codereview.chromium.org/13918002/diff/1/src/ots.cc File src/ots.cc (right): https://codereview.chromium.org/13918002/diff/1/src/ots.cc#newcode415 src/ots.cc:415: std::vector<uint8_t> decompressed_buffer(decompressed_size); On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > * Do we have to zero-fill the buffer? > > Done. std::vector<uint8_t> decompressed_buffer(decompressed_size, 0); and std::vector<uint8_t> decompressed_buffer(decompressed_size); are the same, aren't they? Sorry, I should've been more clear. Please do *NOT* zero-clear the buffer unless it's really necessary. For example, scoped array and new unit8_t[] may suffice. > > * Please define the max decompressed size and return with OTS_FAILURE() when > > |decompressed_size| exceeds that. With the current code, I guess drive-by web > > can easily crash the renderer by oom. > > Good catch, thanks. > I set the limit to 30MB, that is somewhat stricter than other formats where sum > of decompressed tables must be <=30MB. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc File src/woff2.cc (right): https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode157 src/woff2.cc:157: dst[offset] = x >> 24; On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > is there any reason not to check the buffer length on each write? > > I don't think so. > Probably I should create 'writer' version of the Buffer class and use it across > the code? (It requires a considerable amount of change though.) Please add a TODO. I guess you have to do this anyway to pass the security review.. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode181 src/woff2.cc:181: if (n_points > in_size) { On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > Is this correct? I guess this might be >=. > > Correct, I think. > Note that each point consumes 1-4 bytes of |in| buffer, so this is an early > return with lower-bound check. Please add a code comment then. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode506 src/woff2.cc:506: std::vector<uint32_t> loca_values(num_glyphs + 1); On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > Is zero-fill required? > > No - all element should be written. Please fix. it's still zero-filled. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode530 src/woff2.cc:530: if (instruction_size + 2 > glyf_dst_size - glyph_size) { On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > integer overflow not checked? > |instruction_size| is set by Read255UShort, so > 0 <= instruction_size < 65536. add comment please. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode596 src/woff2.cc:596: instruction_size + 2) { On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > integer overflow not checked. > > Same as above. add comment please. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode653 src/woff2.cc:653: if (static_cast<uint64_t>(glyf_table->dst_offset + glyf_table->dst_length) > On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > what is the case for? Looks no-op to me. > > You mean the static_cast is no-op? yes. s/case/cast/. cast<64b>(32b+32b) does not make sense to me because the inner 32b+32b yields 32b result. cast<64b>(32b)+32b might make sense, though. The result of 64b+32b is 64b. For more details, please check the spec. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode741 src/woff2.cc:741: const uint32_t known_tags[29] = { On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > kKnownTags. Move this to around line 69. I guess you can get rid of '29'. > > Done. "Move this to around line 69." is not yet done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode839 src/woff2.cc:839: uint32_t total_length; On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > IIRC, always adding '= 0;' would be better to avoid gcc warnings. please fix > > this and elsewhere. > > Hmm gcc -Wall does not generate any warnings on this? On Chrome OS build environment (gcc 4.7, soon 4.8), it might. FYI. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode860 src/woff2.cc:860: // TODO(bashi): Should call IsValidVersionTag() here. On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > File a bug? > > Now it's easy, as this is a change to ots repository. > Done. nice. https://codereview.chromium.org/13918002/diff/14001/src/woff2.cc File src/woff2.cc (right): https://codereview.chromium.org/13918002/diff/14001/src/woff2.cc#newcode93 src/woff2.cc:93: , flags(0) this does not look like a Chromium style. Please check the style guide. http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr...
Thanks for your review! https://codereview.chromium.org/13918002/diff/1/src/ots.cc File src/ots.cc (right): https://codereview.chromium.org/13918002/diff/1/src/ots.cc#newcode415 src/ots.cc:415: std::vector<uint8_t> decompressed_buffer(decompressed_size); On 2013/05/07 06:11:27, Yusuke Sato wrote: > On 2013/04/23 06:36:26, ksakamoto wrote: > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > * Do we have to zero-fill the buffer? > > > > Done. > > std::vector<uint8_t> decompressed_buffer(decompressed_size, 0); and > std::vector<uint8_t> decompressed_buffer(decompressed_size); are the same, > aren't they? > > Sorry, I should've been more clear. Please do *NOT* zero-clear the buffer unless > it's really necessary. For example, scoped array and new unit8_t[] may suffice. > Oh, got it. We should zero clear this buffer, because ConvertWOFF2ToTTF() does not overwrite padding-bytes. > > > * Please define the max decompressed size and return with OTS_FAILURE() when > > > |decompressed_size| exceeds that. With the current code, I guess drive-by > web > > > can easily crash the renderer by oom. > > > > Good catch, thanks. > > I set the limit to 30MB, that is somewhat stricter than other formats where > sum > > of decompressed tables must be <=30MB. > https://codereview.chromium.org/13918002/diff/1/src/woff2.cc File src/woff2.cc (right): https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode157 src/woff2.cc:157: dst[offset] = x >> 24; On 2013/05/07 06:11:27, Yusuke Sato wrote: > On 2013/04/23 06:36:26, ksakamoto wrote: > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > is there any reason not to check the buffer length on each write? > > > > I don't think so. > > Probably I should create 'writer' version of the Buffer class and use it > across > > the code? (It requires a considerable amount of change though.) > > Please add a TODO. I guess you have to do this anyway to pass the security > review.. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode181 src/woff2.cc:181: if (n_points > in_size) { On 2013/05/07 06:11:27, Yusuke Sato wrote: > On 2013/04/23 06:36:26, ksakamoto wrote: > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > Is this correct? I guess this might be >=. > > > > Correct, I think. > > Note that each point consumes 1-4 bytes of |in| buffer, so this is an early > > return with lower-bound check. > > Please add a code comment then. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode506 src/woff2.cc:506: std::vector<uint32_t> loca_values(num_glyphs + 1); On 2013/05/07 06:11:27, Yusuke Sato wrote: > On 2013/04/23 06:36:26, ksakamoto wrote: > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > Is zero-fill required? > > > > No - all element should be written. > > Please fix. it's still zero-filled. Changed to use vector::reserve for allocation and vector::push_back for filling. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode530 src/woff2.cc:530: if (instruction_size + 2 > glyf_dst_size - glyph_size) { On 2013/05/07 06:11:27, Yusuke Sato wrote: > On 2013/04/23 06:36:26, ksakamoto wrote: > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > integer overflow not checked? > > |instruction_size| is set by Read255UShort, so > > 0 <= instruction_size < 65536. > > add comment please. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode596 src/woff2.cc:596: instruction_size + 2) { On 2013/05/07 06:11:27, Yusuke Sato wrote: > On 2013/04/23 06:36:26, ksakamoto wrote: > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > integer overflow not checked. > > > > Same as above. > > add comment please. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode653 src/woff2.cc:653: if (static_cast<uint64_t>(glyf_table->dst_offset + glyf_table->dst_length) > On 2013/05/07 06:11:27, Yusuke Sato wrote: > On 2013/04/23 06:36:26, ksakamoto wrote: > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > what is the case for? Looks no-op to me. > > > > You mean the static_cast is no-op? > > yes. s/case/cast/. cast<64b>(32b+32b) does not make sense to me because the > inner 32b+32b yields 32b result. cast<64b>(32b)+32b might make sense, though. > The result of 64b+32b is 64b. For more details, please check the spec. Thanks for the explanation. Changed to cast<64b>(32b)+32b. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode657 src/woff2.cc:657: if (static_cast<uint64_t>(loca_table->dst_offset + loca_table->dst_length) > On 2013/04/17 18:38:46, Yusuke Sato wrote: > the same. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode741 src/woff2.cc:741: const uint32_t known_tags[29] = { On 2013/05/07 06:11:27, Yusuke Sato wrote: > On 2013/04/23 06:36:26, ksakamoto wrote: > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > kKnownTags. Move this to around line 69. I guess you can get rid of '29'. > > > > Done. > > "Move this to around line 69." is not yet done. > Oops, sorry. Done. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode839 src/woff2.cc:839: uint32_t total_length; On 2013/05/07 06:11:27, Yusuke Sato wrote: > On 2013/04/23 06:36:26, ksakamoto wrote: > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > IIRC, always adding '= 0;' would be better to avoid gcc warnings. please fix > > > this and elsewhere. > > > > Hmm gcc -Wall does not generate any warnings on this? > > On Chrome OS build environment (gcc 4.7, soon 4.8), it might. FYI. Gcc 4.7 didn't warn for me... I would like to leave them uninitialized for now. Let me know if you are sure that this generates warning and we should avoid it. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode880 src/woff2.cc:880: // Note: change below to ReadLongDirectory to enable long format. On 2013/04/23 06:36:26, ksakamoto wrote: > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > what's "long format"? Don't we have to support it? Please add a comment for a > > newbie like me. Please file a bug if needed. > > I guess we don't. Checking with David. Confirmed that the long format is no longer used. Removed the comment. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode986 src/woff2.cc:986: if (static_cast<uint64_t>(table->dst_offset + transform_length) > On 2013/04/17 18:38:46, Yusuke Sato wrote: > no-op? Fixed. https://codereview.chromium.org/13918002/diff/14001/src/woff2.cc File src/woff2.cc (right): https://codereview.chromium.org/13918002/diff/14001/src/woff2.cc#newcode93 src/woff2.cc:93: , flags(0) On 2013/05/07 06:11:27, Yusuke Sato wrote: > this does not look like a Chromium style. Please check the style guide. > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Constr... Fixed, thanks.
LGTM still stands. https://codereview.chromium.org/13918002/diff/1/src/woff2.cc File src/woff2.cc (right): https://codereview.chromium.org/13918002/diff/1/src/woff2.cc#newcode839 src/woff2.cc:839: uint32_t total_length; On 2013/05/07 10:17:23, ksakamoto wrote: > On 2013/05/07 06:11:27, Yusuke Sato wrote: > > On 2013/04/23 06:36:26, ksakamoto wrote: > > > On 2013/04/17 18:38:46, Yusuke Sato wrote: > > > > IIRC, always adding '= 0;' would be better to avoid gcc warnings. please > fix > > > > this and elsewhere. > > > > > > Hmm gcc -Wall does not generate any warnings on this? > > > > On Chrome OS build environment (gcc 4.7, soon 4.8), it might. FYI. > > Gcc 4.7 didn't warn for me... > I would like to leave them uninitialized for now. Let me know if you are sure > that this generates warning and we should avoid it. Please run 'cros_amd64' trybot when you update Chrome's DEPS.
Message was sent while issue was closed.
Committed patchset #4 manually as r100 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
