Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681733008:
Improve error handling of the transport security state generator. (Closed)
DescriptionImprove error handling of the transport security state generator.
Split off from https://codereview.chromium.org/2660793002.
This is the first part of a larger CL that improves the error handling of the
generator. The old code would often (D)CHECK on incorrect inputs which isn't
ideal. This CL replaces the CHECKs with boolean return values and outputs human
readable errors when something goes wrong.
Also fixes a small bug, incorrect comments, and removes unused code.
BUG=595493
Review-Url: https://codereview.chromium.org/2681733008
Cr-Commit-Position: refs/heads/master@{#453901}
Committed: https://chromium.googlesource.com/chromium/src/+/0a16d63e9fc212b9a4c2e3d01cc036745880f516
Patch Set 1 #
Total comments: 24
Patch Set 2 #Patch Set 3 : Remove ErrorList #Messages
Total messages: 26 (16 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by martijn@martijnc.be to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:40001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Improve error handling of transport security state generator. BUG=595493 ========== to ========== Improve error handling of the transport security state generator. Split off from https://codereview.chromium.org/2660793002. This is the first part of a larger CL that improves the error handling of the generator. The old code would often (D)CHECK on incorrect inputs which isn't ideal. This CL replaces the CHECKs with boolean return values and outputs human readable errors when something goes wrong. Also fixes a small bug, incorrect comments, and removes unused code. BUG=595493 ==========
martijn@martijnc.be changed reviewers: + lgarron@chromium.org, rsleevi@chromium.org
Description was changed from ========== Improve error handling of the transport security state generator. Split off from https://codereview.chromium.org/2660793002. This is the first part of a larger CL that improves the error handling of the generator. The old code would often (D)CHECK on incorrect inputs which isn't ideal. This CL replaces the CHECKs with boolean return values and outputs human readable errors when something goes wrong. Also fixes a small bug, incorrect comments, and removes unused code. BUG=595493 ========== to ========== Improve error handling of the transport security state generator. Split off from https://codereview.chromium.org/2660793002. This is the first part of a larger CL that improves the error handling of the generator. The old code would often (D)CHECK on incorrect inputs which isn't ideal. This CL replaces the CHECKs with boolean return values and outputs human readable errors when something goes wrong. Also fixes a small bug, incorrect comments, and removes unused code. BUG=595493 ==========
Hi, this is the first group of changes I split of from the large CL. Can you review or would you prefer I send this to another reviewer? Thanks!
There's probably still opportunity to split this into smaller CLs, and to justify each CL individually :) A lot of comments below - sorry for the delay. https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/bit_writer.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/bit_writer.cc:36: position_ += (8 - used_); This seems like a non-trivial behaviour change. If I understand correctly, the old API contract was that you MUST write bits along an 8 bit boundary (because they only flush on the 8th bit). By introducing this, it allows partial advancement of non-aligned bits, except for line 37 still writes fully aligned bits. I'm not saying it's wrong; it just subtle and I'm hoping you can explain more the motivation for this. This also seems small enough that it could be its own CL, with the commit description explaining this bug here? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/huffman/huffman_builder.cc (left): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder.cc:125: if (item.second > 0) { Why this change? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/huffman/huffman_builder.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder.cc:104: DCHECK(child_position < 512) << "huffman tree too large"; What's the motivation for demoting this? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/preloaded_state_generator.h (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/preloaded_state_generator.h:32: // false on failure. Why the API signature change? Wouldn't returning an empty string be sufficient to signal error? Why or why not? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/transport_security_state_generator.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:40: using ErrorList = std::vector<std::string>; Why error list instead of VLOG? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:61: } This definitely feels like a cheat around the style guide, both around Streams ( https://google.github.io/styleguide/cppguide.html#Streams ) and to_string() being forbidden. Luckily, there's a non-cheaty way :) https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h... https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:93: success = false; Why do you keep processing when success is false? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:102: success = false; Why do you keep processing when success is false? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/trie/trie_bit_buffer.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/trie/trie_bit_buffer.cc:81: } Why the change from DCHECK to being a soft-check? What's the motivation? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/trie/trie_bit_buffer.h (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/trie/trie_bit_buffer.h:54: // will occur. Why this semantic change of the API? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/trie/trie_writer.h (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/trie/trie_writer.h:42: // |buffer_|. Returns the position of the trie root. Out of date comment
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
Thanks for the feedback! I've split off a couple of changes (indicated in the comments). https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/bit_writer.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/bit_writer.cc:36: position_ += (8 - used_); On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > This seems like a non-trivial behaviour change. > > If I understand correctly, the old API contract was that you MUST write bits along an 8 bit boundary (because they only flush on the 8th bit). > > By introducing this, it allows partial advancement of non-aligned bits, except for line 37 still writes fully aligned bits. > > I'm not saying it's wrong; it just subtle and I'm hoping you can explain more the motivation for this. This doesn't change the API contract, both the old and the new code allow writing of non-aligned bits over the byte boundary. The old code has a bug where |position_| no longer matches the actual position after Flush() is called because Flush() writes up to 8 bits to buffer but doesn't update |position_|. The generator never writes to a BitWriter after it got flushed so doesn't hit this bug, but it is possible so seems worth fixing. > This also seems small enough that it could be its own CL, with the commit description explaining this bug here? Moved to https://codereview.chromium.org/2700433004 https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/huffman/huffman_builder.cc (left): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder.cc:125: if (item.second > 0) { On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > Why this change? This check is always true. Only RecordUsage() writes to |counts_| and only increments the value. If a character is never used it won't be in |counts_| so the code wouldn't iterate over it. If a character is used, its count would be 1 or higher. Moved to https://codereview.chromium.org/2694363003 https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/huffman/huffman_builder.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/huffman/huffman_builder.cc:104: DCHECK(child_position < 512) << "huffman tree too large"; On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > What's the motivation for demoting this? The style guide says "Use CHECK() if the consequence of a failed assertion would be a security vulnerability, where crashing the browser is preferable." I don't think that's the case here. When I ported the old Go script, I initially used CHECK where the Go script panic()'ed (and later changed some to DCHECK). That was a bad idea and the cause for most of the changes in the CL. Moved to https://codereview.chromium.org/2694363003 https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/preloaded_state_generator.h (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/preloaded_state_generator.h:32: // false on failure. On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > Why the API signature change? Wouldn't returning an empty string be sufficient to signal error? Why or why not? Yes but felt the boolean indicated that a bit better. Updated to return the empty string on failure. https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/transport_security_state_generator.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:40: using ErrorList = std::vector<std::string>; On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > Why error list instead of VLOG? With this I can print the errors directly to the console. I'm not very familiar with VLOG but the documentation indicates you need to pass additional parameters (and on Windows you need Sawbuck) the get VLOG output. I think it is easier here to just print the errors immediately. Having the tool fail during the build (or when it is executed manually) without error output might be confusing? https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:61: } On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > This definitely feels like a cheat around the style guide, both around Streams ( https://google.github.io/styleguide/cppguide.html#Streams ) and to_string() being forbidden. > > Luckily, there's a non-cheaty way :) > > https://cs.chromium.org/chromium/src/base/strings/string_number_conversions.h... Thanks, fixed. https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:93: success = false; On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > Why do you keep processing when success is false? I wanted to print a list of all non-fatal errors rather than just the first but it might not be worth the additional code complexity. Removed this logic. https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:102: success = false; On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > Why do you keep processing when success is false? Removed (as above). https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/trie/trie_bit_buffer.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/trie/trie_bit_buffer.cc:81: } On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > Why the change from DCHECK to being a soft-check? What's the motivation? Not crashing on incorrect inputs. But looking at this again the DCHECK was probably more appropriate because the HuffmanRepresentationTable is generated by the code based on the (user) input. Missing characters thus indicate an error in the code and should never happen, even with incorrect input. Reverted to the old behavior. https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/trie/trie_bit_buffer.h (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/trie/trie_bit_buffer.h:54: // will occur. On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > Why this semantic change of the API? The old comment was incorrect. The API already behaves this way. https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/trie/trie_writer.h (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/trie/trie_writer.h:42: // |buffer_|. Returns the position of the trie root. On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > Out of date comment Updated.
Mostly LG, but one design question below :) https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/transport_security_state_generator.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:40: using ErrorList = std::vector<std::string>; On 2017/02/15 22:07:59, martijnc wrote: > On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > > Why error list instead of VLOG? > > With this I can print the errors directly to the console. I'm not very familiar > with VLOG but the documentation indicates you need to pass additional parameters > (and on Windows you need Sawbuck) the get VLOG output. Windows wouldn't require Sawbuck here - VLOG will dump to stderr, but yes, it requires you specify the vlog level (--v=1 / --v=2, etc) > I think it is easier here to just print the errors immediately. Having the tool > fail during the build (or when it is executed manually) without error output > might be confusing? So that's a really good argument about "Why not VLOG" - since this is part of the build process and clearly communicating errors is good - but I guess I'm then confused why the ErrorList rather than splitting to std::cerr directly, since this is a command-line tool generating things. Or using the more general pattern, why not just LOG(ERROR) << stuff and use that pattern? I'm trying to understand the design decision/rationale a bit more, since what you have is technically valid, but it seems to add a degree of complexity and diversion from common patterns that I just wanted to make sure I understood, and to see if there were any simpler solutions (or if they'd been ruled out)
https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... File net/tools/transport_security_state_generator/transport_security_state_generator.cc (right): https://codereview.chromium.org/2681733008/diff/60001/net/tools/transport_sec... net/tools/transport_security_state_generator/transport_security_state_generator.cc:40: using ErrorList = std::vector<std::string>; On 2017/02/16 at 01:50:40, Ryan Sleevi wrote: > On 2017/02/15 22:07:59, martijnc wrote: > > On 2017/02/14 at 20:53:17, Ryan Sleevi wrote: > > > Why error list instead of VLOG? > > > > With this I can print the errors directly to the console. I'm not very familiar > > with VLOG but the documentation indicates you need to pass additional parameters > > (and on Windows you need Sawbuck) the get VLOG output. > > Windows wouldn't require Sawbuck here - VLOG will dump to stderr, but yes, it requires you specify the vlog level (--v=1 / --v=2, etc) > > > I think it is easier here to just print the errors immediately. Having the tool > > fail during the build (or when it is executed manually) without error output > > might be confusing? > > So that's a really good argument about "Why not VLOG" - since this is part of the build process and clearly communicating errors is good - but I guess I'm then confused why the ErrorList rather than splitting to std::cerr directly, since this is a command-line tool generating things. > > Or using the more general pattern, why not just LOG(ERROR) << stuff and use that pattern? > > I'm trying to understand the design decision/rationale a bit more, since what you have is technically valid, but it seems to add a degree of complexity and diversion from common patterns that I just wanted to make sure I understood, and to see if there were any simpler solutions (or if they'd been ruled out) I want to avoid having multiple places/files that write output directly. My thinking was to only write output from the main cc file (transport_security_state_generator.cc). That puts this file in charge of all output and ensures other files don't generate noise. Although the functions that use ErrorList are currently in the main cc file, they'll have to move to a separate file so that they can be tested as I've done in [1]. That was my reasoning for using ErrorList but I'm happy to revert this or use VLOG(ERROR) if that puts it in line with the general Chromium style. [1] https://codereview.chromium.org/2660793002
On 2017/02/16 18:02:46, martijnc wrote: > I want to avoid having multiple places/files that write output directly. My > thinking was to only write output from the main cc file > (transport_security_state_generator.cc). That puts this file in charge of all > output and ensures other files don't generate noise. > > Although the functions that use ErrorList are currently in the main cc file, > they'll have to move to a separate file so that they can be tested as I've done > in [1]. > > That was my reasoning for using ErrorList but I'm happy to revert this or use > VLOG(ERROR) if that puts it in line with the general Chromium style. > > [1] https://codereview.chromium.org/2660793002 Apologies, somehow I must have marked your code review email read, but not responded. Yeah, for a case like a command-line tool, I think [D/V]LOG(ERROR) is more consistent with the approaches we see for command-line tooling. The big question is whether or not the error is part of the API or not, but I think in the case of ErrorList, it's pretty clearly not a part of the API (e.g. it's not defined codes being stringized) So I think using LOG(ERROR) -- even liberally sprinkled through - is a good bet. The other question mostly relates to unit tests and whether the unittests are generating output for 'tested' cases that is unnecessary. That's where the VLOG comment came from, but I think having looked at it closer as its used, even if some of these are moved to other files, you should be safe with the LOG(ERROR) approach. Unfortunately, I can't find a good 'style' cite for that, other than what I've seen in the code both inside Google and inside Chrome.
Patchset #3 (id:140001) has been deleted
On 2017/02/28 at 04:09:07, rsleevi wrote: > Apologies, somehow I must have marked your code review email read, but not responded. > > Yeah, for a case like a command-line tool, I think [D/V]LOG(ERROR) is more consistent with the approaches we see for command-line tooling. The big question is whether or not the error is part of the API or not, but I think in the case of ErrorList, it's pretty clearly not a part of the API (e.g. it's not defined codes being stringized) > > So I think using LOG(ERROR) -- even liberally sprinkled through - is a good bet. > > The other question mostly relates to unit tests and whether the unittests are generating output for 'tested' cases that is unnecessary. That's where the VLOG comment came from, but I think having looked at it closer as its used, even if some of these are moved to other files, you should be safe with the LOG(ERROR) approach. > > Unfortunately, I can't find a good 'style' cite for that, other than what I've seen in the code both inside Google and inside Chrome. Thanks for the feedback. I've uploaded a new patchset which replaces ErrorList with LOG(ERROR) and the -v(erbose) option with VLOG().
lgtm
The CQ bit was checked by martijn@martijnc.be
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1488365147888830,
"parent_rev": "d4809ef025d50acb50489134273e13444252a8e6", "commit_rev":
"0a16d63e9fc212b9a4c2e3d01cc036745880f516"}
Message was sent while issue was closed.
Description was changed from ========== Improve error handling of the transport security state generator. Split off from https://codereview.chromium.org/2660793002. This is the first part of a larger CL that improves the error handling of the generator. The old code would often (D)CHECK on incorrect inputs which isn't ideal. This CL replaces the CHECKs with boolean return values and outputs human readable errors when something goes wrong. Also fixes a small bug, incorrect comments, and removes unused code. BUG=595493 ========== to ========== Improve error handling of the transport security state generator. Split off from https://codereview.chromium.org/2660793002. This is the first part of a larger CL that improves the error handling of the generator. The old code would often (D)CHECK on incorrect inputs which isn't ideal. This CL replaces the CHECKs with boolean return values and outputs human readable errors when something goes wrong. Also fixes a small bug, incorrect comments, and removes unused code. BUG=595493 Review-Url: https://codereview.chromium.org/2681733008 Cr-Commit-Position: refs/heads/master@{#453901} Committed: https://chromium.googlesource.com/chromium/src/+/0a16d63e9fc212b9a4c2e3d01cc0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0a16d63e9fc212b9a4c2e3d01cc0... |
