net: add CRL filter infrastructure.

net: add CRL filter infrastructure.

This doesn't plumb anything in yet, it just runs unittests.

BUG=none
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87676

[Mike Belshe, 2011-06-02 18:11:25]

lgtm

Just a few nits.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc
File net/base/crl_filter.cc (right):

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode295
net/base/crl_filter.cc:295: uint8 r = kDeBruijnLookup[((w&-w) * kDeBruijn) >>
27];
nit: add spacing between operators:  (w & -w)

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode645
net/base/crl_filter.cc:645: if (header_len < 0 || header_len >
kMaxHeaderLengthBytes)
You could use DCHECKs or CHECKs here.  If we aren't passing the sanity tests, it
could be indicative of something really bad.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter_unittest.cc
File net/base/crl_filter_unittest.cc (right):

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter_unittest.cc...
net/base/crl_filter_unittest.cc:8: static const char kTestFilter[] = {
nit:  how did you generate the filter?

instructions will help others that work on this.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter_unittest.cc...
net/base/crl_filter_unittest.cc:167: std::string(kDeltaResultSHA256,
sizeof(kDeltaResultSHA256)));
nit:  should you test some CheckCertificate() calls before/after the updated
deltas?

[agl, 2011-06-02 21:04:39]

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc
File net/base/crl_filter.cc (right):

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode295
net/base/crl_filter.cc:295: uint8 r = kDeBruijnLookup[((w&-w) * kDeBruijn) >>
27];
On 2011/06/02 18:11:25, Mike Belshe wrote:
> nit: add spacing between operators:  (w & -w)

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode645
net/base/crl_filter.cc:645: if (header_len < 0 || header_len >
kMaxHeaderLengthBytes)
On 2011/06/02 18:11:25, Mike Belshe wrote:
> You could use DCHECKs or CHECKs here.  If we aren't passing the sanity tests,
it
> could be indicative of something really bad.

I've added a NOTREACHED. I didn't want to add CHECKs in case a bad CRL filter
push causes every Chrome to crash!

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter_unittest.cc
File net/base/crl_filter_unittest.cc (right):

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter_unittest.cc...
net/base/crl_filter_unittest.cc:8: static const char kTestFilter[] = {
On 2011/06/02 18:11:25, Mike Belshe wrote:
> nit:  how did you generate the filter?

I've added a comment, although it just directs people to contact me. At the
moment the code for generating the CRL filters is still in development and needs
patched versions of all sorts of things.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter_unittest.cc...
net/base/crl_filter_unittest.cc:167: std::string(kDeltaResultSHA256,
sizeof(kDeltaResultSHA256)));
On 2011/06/02 18:11:25, Mike Belshe wrote:
> nit:  should you test some CheckCertificate() calls before/after the updated
> deltas?

If the SHA256 matches then that's ok for now. It's quite a lot of work to get
the SPKI hash etc and I'll probably save that for when I redo all the golden
data in here.

[Mike Belshe, 2011-06-02 21:17:12]

On 2011/06/02 21:04:39, agl wrote:
> http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc
> File net/base/crl_filter.cc (right):
> 
>
http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode295
> net/base/crl_filter.cc:295: uint8 r = kDeBruijnLookup[((w&-w) * kDeBruijn) >>
> 27];
> On 2011/06/02 18:11:25, Mike Belshe wrote:
> > nit: add spacing between operators:  (w & -w)
> 
> Done.
> 
>
http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode645
> net/base/crl_filter.cc:645: if (header_len < 0 || header_len >
> kMaxHeaderLengthBytes)
> On 2011/06/02 18:11:25, Mike Belshe wrote:
> > You could use DCHECKs or CHECKs here.  If we aren't passing the sanity
tests,
> it
> > could be indicative of something really bad.
> 
> I've added a NOTREACHED. I didn't want to add CHECKs in case a bad CRL filter
> push causes every Chrome to crash!
> 
> http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter_unittest.cc
> File net/base/crl_filter_unittest.cc (right):
> 
>
http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter_unittest.cc...
> net/base/crl_filter_unittest.cc:8: static const char kTestFilter[] = {
> On 2011/06/02 18:11:25, Mike Belshe wrote:
> > nit:  how did you generate the filter?
> 
> I've added a comment, although it just directs people to contact me. At the
> moment the code for generating the CRL filters is still in development and
needs
> patched versions of all sorts of things.
> 
>
http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter_unittest.cc...
> net/base/crl_filter_unittest.cc:167: std::string(kDeltaResultSHA256,
> sizeof(kDeltaResultSHA256)));
> On 2011/06/02 18:11:25, Mike Belshe wrote:
> > nit:  should you test some CheckCertificate() calls before/after the updated
> > deltas?
> 
> If the SHA256 matches then that's ok for now. It's quite a lot of work to get
> the SPKI hash etc and I'll probably save that for when I redo all the golden
> data in here.

OK - when you get the tools for generating data checked in, maybe an end-to-end
test where you generate data, verify the filter, patch the data, and then verify
again would be good.  That would embody the entire flow...

LGTM

[Ryan Sleevi, 2011-06-02 22:01:54]

agl: two LEAKs and a BUG below, along with a host of nits that , in the fine
tradition of nits, are minor, pedantic, and not entirely critical.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc
File net/base/crl_filter.cc (right):

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode47
net/base/crl_filter.cc:47: return true;
LEAK: inflateEnd() is never called, particularly along the early termination
cases.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode110
net/base/crl_filter.cc:110: RangeDecoder(base::StringPiece in, const
std::vector<uint32> spans)
nit: const std::vector<uint32> -> const std::vector<uint32>&

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode123
net/base/crl_filter.cc:123: while (high_ >> 24 == low_ >> 24) {
nit: I think parenthesis for the sub-statements here and on lines 146 and 161
would help readability, even though operator precedence doesn't require them.
Not required by the style guide though.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode139
net/base/crl_filter.cc:139: for (unsigned i = 0; i < spans_.size(); i++) {
nit: unsigned -> size_t (or std::vector<uint32>::size_type for the extremely
pedantic)

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode178
net/base/crl_filter.cc:178: const std::vector<uint32> spans_;
nit?: const std::vector<uint32>& , to further avoid any copies

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode209
net/base/crl_filter.cc:209: iterator(base::StringPiece data, unsigned
num_values)
nit?: unsigned -> size_t || uint32 || uint64

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode324
net/base/crl_filter.cc:324: uint32 w;
nit?: Move down two lines, between line 326 and 327

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode356
net/base/crl_filter.cc:356: 
nit: Delete this blank line

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode402
net/base/crl_filter.cc:402: if (num_bits_ > 0) {
nit: Inconsistent bracing with rest of the file's one-line statements (eg: see
lines 378/379 or 382/383)

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode430
net/base/crl_filter.cc:430: uint8* buf_;
LEAK: buf_ is leaked (allocated on lines 410 || 413)

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode432
net/base/crl_filter.cc:432: unsigned buf_used_;
nit: size_t for bytes in memory,
http://www.chromium.org/developers/coding-style#TOC-Types

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode434
net/base/crl_filter.cc:434: unsigned num_bits_;

DISALLOW_COPY_AND_ASSIGN(BitWriter)

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode572
net/base/crl_filter.cc:572: for (size_t i = 0; i < crls_included->GetSize();
i++) {
world's smallest nit, part 2: ++i

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode590
net/base/crl_filter.cc:590: url,
nit: 2 spaces -> 4 spaces

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode714
net/base/crl_filter.cc:714: static const unsigned num_span_values = 3;
nit: constant name

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode715
net/base/crl_filter.cc:715: if (data.size() < num_span_values * sizeof(uint32))
nit: Use a variable for num_span_values * sizeof(uint32), since you compute it
here and again on lines 719/720

Given that num_span_values is itself a constant, you could make this a constant
as well for readability.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode769
net/base/crl_filter.cc:769: if (crls_included_.count(std::make_pair<std::string,
std::string>(
nit: base/stl_util-inl.h has ContainsKey, which is the more prevalent pattern
within the net/ code

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h
File net/base/crl_filter.h (right):

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode41
net/base/crl_filter.h:41: 
BUG: No constructors defined, so an implicit (public) constructor will be
created. Given the static method Parse(), I would think the ctor was meant to be
private (and explicitly specified)

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode42
net/base/crl_filter.h:42: ~CRLFilter();
If it is RefCounted, shouldn't this be private:, with the Refcounted<CRLFilter>
friend'ed, to prevent both stack allocations and delete's?

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode59
net/base/crl_filter.h:59: base::StringPiece parent_spki);
nit?: Result CheckCertificate(...) const


nit?: base::StringPiece -> const base::StringPiece&
Admittedly, there's no copying of the underlying data going on, but I thought
passing objects by value was something discouraged? Also throughout the .cc file

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode63
net/base/crl_filter.h:63: CRLFilter* ApplyDelta(base::StringPiece delta_bytes);
suggestion: CRLFilter* ApplyDelta(...) const ? Since it returns a new CRLFilter?

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode71
net/base/crl_filter.h:71: // DebugValues return all GCS values, in order. This
should only be used
nit: First appearance of "GCS" in the header, without comment that it's a
GolombCompressedSet.

See also
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode83
net/base/crl_filter.h:83: 
nit: For these "Should only be used for testing", would it be better to make
them private and friended to the test? That is the dominant pattern in the net/
code

Or even just friend the test, so it can access the data members directly, and
move these helpers into the test file.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode90
net/base/crl_filter.h:90: };
nit: Private, unnamed enum can be moved wholly into the implementation file - no
need for inclusion in the header.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode92
net/base/crl_filter.h:92: static CRLFilter*
CRLFilterFromHeader(base::StringPiece header);
nit: Static private function can be moved wholly into the implementation file -
no need for inclusion in the header.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode94
net/base/crl_filter.h:94: const std::string& parent_spki_sha256);
nit?: bool CRLIsCovered(...) const
nit: std::vector<base::StringPiece> crl_urls -> const
std::vector<base::StringPiece>& crl_urls

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode96
net/base/crl_filter.h:96: int64 not_before_, not_after_;
super-minor nit?: I thought the style guide prohibited multiple variables from
being declared in the same statement, but I'm having trouble finding where.
Still, it seems uncommon in the code that I've seen.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode98
net/base/crl_filter.h:98: unsigned sequence_;
nit: Explicitly sized integer (presumably, int64 based on the JSON
implementation) 

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Integer_Types
http://www.chromium.org/developers/coding-style#TOC-Types

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode99
net/base/crl_filter.h:99: unsigned num_entries_;
nit: size_t

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.h#newcode105
net/base/crl_filter.h:105: scoped_ptr<GolombCompressedSet> gcs_;

DISALLOW_COPY_AND_ASSIGN(CRLFilter)

http://codereview.chromium.org/6965015/diff/7001/net/base/crl_filter_unittest.cc
File net/base/crl_filter_unittest.cc (right):

http://codereview.chromium.org/6965015/diff/7001/net/base/crl_filter_unittest...
net/base/crl_filter_unittest.cc:161: base::StringPiece s(reinterpret_cast<const
char *>(kDeltaTestFilter1),
nit: const char * -> const char*

http://codereview.chromium.org/6965015/diff/7001/net/base/crl_filter_unittest...
net/base/crl_filter_unittest.cc:173: std::string(reinterpret_cast<const char
*>(kDeltaResultSHA256),
nit: const char * -> const char*

[agl, 2011-06-02 22:57:17]

Thanks! I was already working on the memory leaks because of the valgrind
builder. I also switched to returning scoped_refptrs rather than raw pointers to
make sure that there's no confusion about the ownership.

I'm afraid that I've been writing too much Go and the compiler can only tell me
when I've missed semicolons!

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc
File net/base/crl_filter.cc (right):

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode47
net/base/crl_filter.cc:47: return true;
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> LEAK: inflateEnd() is never called, particularly along the early termination
> cases. 

Opps, thanks!

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode110
net/base/crl_filter.cc:110: RangeDecoder(base::StringPiece in, const
std::vector<uint32> spans)
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: const std::vector<uint32> -> const std::vector<uint32>& 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode123
net/base/crl_filter.cc:123: while (high_ >> 24 == low_ >> 24) {
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: I think parenthesis for the sub-statements here and on lines 146 and 161
> would help readability, even though operator precedence doesn't require them.
> Not required by the style guide though. 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode139
net/base/crl_filter.cc:139: for (unsigned i = 0; i < spans_.size(); i++) {
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: unsigned -> size_t (or std::vector<uint32>::size_type for the extremely
> pedantic) 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode178
net/base/crl_filter.cc:178: const std::vector<uint32> spans_;
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit?: const std::vector<uint32>& , to further avoid any copies

I'd rather reduce the surprise than save copying 12 bytes. Stashing a pointer
which was a const reference is a bit of a sharp edge.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode324
net/base/crl_filter.cc:324: uint32 w;
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit?: Move down two lines, between line 326 and 327 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode356
net/base/crl_filter.cc:356: 
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: Delete this blank line 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode402
net/base/crl_filter.cc:402: if (num_bits_ > 0) {
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: Inconsistent bracing with rest of the file's one-line statements (eg: see
> lines 378/379 or 382/383) 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode430
net/base/crl_filter.cc:430: uint8* buf_;
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> LEAK: buf_ is leaked (allocated on lines 410 || 413) 

Thanks!

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode432
net/base/crl_filter.cc:432: unsigned buf_used_;
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: size_t for bytes in memory,
> http://www.chromium.org/developers/coding-style#TOC-Types 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode434
net/base/crl_filter.cc:434: unsigned num_bits_;
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> 
> DISALLOW_COPY_AND_ASSIGN(BitWriter)

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode590
net/base/crl_filter.cc:590: url,
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: 2 spaces -> 4 spaces 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode714
net/base/crl_filter.cc:714: static const unsigned num_span_values = 3;
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: constant name
> 
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Constant_Names 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode715
net/base/crl_filter.cc:715: if (data.size() < num_span_values * sizeof(uint32))
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: Use a variable for num_span_values * sizeof(uint32), since you compute it
> here and again on lines 719/720
> 
> Given that num_span_values is itself a constant, you could make this a
constant
> as well for readability. 

Done.

http://codereview.chromium.org/6965015/diff/1/net/base/crl_filter.cc#newcode769
net/base/crl_filter.cc:769: if (crls_included_.count(std::make_pair<std::string,
std::string>(
On 2011/06/02 22:01:54, Ryan Sleevi wrote:
> nit: base/stl_util-inl.h has ContainsKey, which is the more prevalent pattern
> within the net/ code 

Done.