Certificate Transparency: Code for unpacking EV cert hashes whitelist

chrome/browser/net/bit_stream_reader.cc

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/net/bit_stream_reader.h"
+
+#include "base/big_endian.h"
+#include "base/logging.h"
+
+namespace internal {
+
+BitStreamReader::BitStreamReader(const base::StringPiece& source)
+    : source_(source), current_byte_(0), current_bit_(7) {
+}
+
+bool BitStreamReader::ReadUnaryEncoding(uint64_t* out) {
+  uint64_t res = 0;
+  if (BitsLeft() == 0)
+    return false;
+
+  while ((BitsLeft() > 0) && ReadBit())
+    res++;

[Ryan Sleevi, 2014/10/20 19:18:24]

Why keep the temporary?

You could change line 20 to
*out = 0;
while (...)
  ++(*out);
return true;

[Eran Messeri, 2014/10/21 14:59:59]

Done.

+
+  *out = res;
+  return true;
+}
+
+bool BitStreamReader::ReadBits(uint8_t num_bits, uint64_t* out) {
+  if (num_bits > 64)
+    return false;

[Ryan Sleevi, 2014/10/20 19:18:24]

API: Your API contract says they shouldn't call with num_bits > 64. Should this
be a DCHECK here (indicating API failure), with some higher-layer doing the >64
comparison (as part of handling untrusted input)

[Eran Messeri, 2014/10/21 14:59:59]

Done. In practice I always read a fixed amount so no untrusted input in this
case.

+
+  if (BitsLeft() < num_bits)
+    return false;
+
+  uint64_t res = 0;
+  for (uint8_t i = 0; i < num_bits; ++i)
+    res |= (static_cast<uint64_t>(ReadBit()) << (num_bits - (i + 1)));
+
+  *out = res;

[Ryan Sleevi, 2014/10/20 19:18:24]

ditto the temporary again

[Eran Messeri, 2014/10/21 14:59:59]

Done.

+  return true;
+}
+
+uint64_t BitStreamReader::BitsLeft() const {
+  if (current_byte_ == source_.length())
+    return 0;
+  return (source_.length() - (current_byte_ + 1)) * 8 + current_bit_ + 1;

[Ryan Sleevi, 2014/10/20 19:18:24]

BUG: integer overflow issues here. Should you use the checked_numerics from
base?

[Eran Messeri, 2014/10/21 14:59:59]

Added a DCHECK that source_.length() is greater than current_byte_.
current_byte_ should never exceed source_.length() and if they are equal (which
is covered by the condition at the beginning of the method) then there are no
more bits to read.

Casting both source_.length() and current_byte_ to signed values just for the
negation would be cumbersome and we can get the same protection against
underflow with the DCHECK.

As for the potential for overflow, I've added a DCHECK in the c'tor that the
source_.length() never exceeds UINT32_MAX. In practice creating a StringPiece
instance so big that it's length could get anywhere near uint64 limit is likely
to result in problems elsewhere...

+}
+
+uint8_t BitStreamReader::ReadBit() {
+  DCHECK_GT(BitsLeft(), 0u);
+  DCHECK(current_bit_ < 8 && current_bit_ >= 0);
+  uint8_t res =
+      (source_.data()[current_byte_] & (1 << current_bit_)) >> current_bit_;
+  current_bit_--;
+  if (current_bit_ < 0) {
+    current_byte_++;
+    current_bit_ = 7;
+  }
+
+  return res;
+}
+
+} // namespace internal