Weave Packet Receiver

components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.cc

+// Copyright 2016 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 "components/proximity_auth/ble/bluetooth_low_energy_weave_packet_receiver.h"
+
+#include "components/proximity_auth/logging/logging.h"
+
+using proximity_auth::BluetoothLowEnergyWeavePacketGenerator;
+
+namespace {
+typedef BluetoothLowEnergyWeavePacketGenerator::PacketType PacketType;
+typedef BluetoothLowEnergyWeavePacketGenerator::ControlCommand ControlCommand;
+typedef BluetoothLowEnergyWeavePacketGenerator::ReasonForClose ReasonForClose;
+
+const uint8_t kMaxPacketCounter = 8;
+const uint16_t kMaxControlPacketSize = 20;
+const uint16_t kMinPacketSize = 20;

[Kyle Horimoto, 2016/06/20 23:25:40]

nit: Please word this constant differently. It's not the minimum packet size -
it's the minimum value for the maximum packet size.

[jingxuy, 2016/06/21 01:19:14]

Done.

[Kyle Horimoto, 2016/06/21 02:10:26]

Haha, MinMax is also very confusing. Can you think of another name?

[Kyle Horimoto, 2016/06/21 18:13:37]

Ping.

[jingxuy, 2016/06/22 01:42:23]

Done.

+const uint16_t kMinConnectionRequestSize = 7;
+const uint16_t kMinConnectionResponseSize = 5;
+const uint16_t kMinConnectionCloseSize = 3;
+const uint16_t kSupportedWeaveVersion = 1;
+
+}  // namespace
+
+namespace proximity_auth {
+
+BluetoothLowEnergyWeavePacketReceiver::Factory*
+    BluetoothLowEnergyWeavePacketReceiver::Factory::factory_instance_ = nullptr;
+
+std::unique_ptr<BluetoothLowEnergyWeavePacketReceiver>
+BluetoothLowEnergyWeavePacketReceiver::Factory::NewInstance(
+    ReceiverType receiver_type) {
+  if (factory_instance_ == nullptr) {
+    factory_instance_ = new Factory();
+  }
+  return std::unique_ptr<BluetoothLowEnergyWeavePacketReceiver>(
+      factory_instance_->BuildInstance(receiver_type));
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::Factory::SetInstanceForTesting(
+    Factory* factory) {
+  factory_instance_ = factory;
+}
+
+BluetoothLowEnergyWeavePacketReceiver*
+BluetoothLowEnergyWeavePacketReceiver::Factory::BuildInstance(
+    ReceiverType receiver_type) {
+  return new BluetoothLowEnergyWeavePacketReceiver(receiver_type);
+}
+
+BluetoothLowEnergyWeavePacketReceiver::BluetoothLowEnergyWeavePacketReceiver(
+    ReceiverType receiver_type)
+    : receiver_type_(receiver_type),
+      packet_number_(0),
+      state_(State::CONNECTING),
+      reason_for_close_(ReasonForClose::CLOSE_WITHOUT_ERROR),
+      reason_to_close_(ReasonForClose::CLOSE_WITHOUT_ERROR) {
+  SetMaxPacketSize(kMinPacketSize);
+}
+
+BluetoothLowEnergyWeavePacketReceiver::
+    ~BluetoothLowEnergyWeavePacketReceiver() {}
+
+BluetoothLowEnergyWeavePacketReceiver::State
+BluetoothLowEnergyWeavePacketReceiver::GetState() {
+  return state_;
+}
+
+uint16_t BluetoothLowEnergyWeavePacketReceiver::GetMaxPacketSize() {
+  // max_packet_size_ is well defined in every state.

[Kyle Horimoto, 2016/06/20 23:25:40]

It doesn't seem like this is true. max_packet_size_ isn't initialized to any
value. I think you should initialize it to 0. However, please make a new
constant for that value instead of using 0 directly.

[jingxuy, 2016/06/21 01:19:14]

It's default to 20 in the SetMaxPacketSize in the constructor

+  return max_packet_size_;
+}
+
+ReasonForClose BluetoothLowEnergyWeavePacketReceiver::GetReasonForClose() {
+  DCHECK(state_ == State::CONNECTION_CLOSED);
+  return reason_for_close_;
+}
+
+ReasonForClose BluetoothLowEnergyWeavePacketReceiver::GetReasonToClose() {
+  DCHECK(state_ == State::ERROR);
+  return reason_to_close_;
+}
+
+std::string BluetoothLowEnergyWeavePacketReceiver::GetDataMessage() {
+  DCHECK(state_ == State::DATA_READY);
+  return std::string(data_message_.begin(), data_message_.end());
+}
+
+BluetoothLowEnergyWeavePacketReceiver::State
+BluetoothLowEnergyWeavePacketReceiver::ReceivePacket(const Packet& packet) {
+  if (packet.empty()) {
+    Error("Empty packet is not a valid uWeave packet.",

[Kyle Horimoto, 2016/06/20 23:25:39]

For these error messages, please make them declarative about what happened. You
can add a secondary sentence if you need more context.

For example, this could be "Received an empty packet, which is not a valid in
the uWeave protocol."

Another example: "Server couldn't process connection response." ==> "Received a
connection response packet; this is invalid when in server mode."

[jingxuy, 2016/06/21 01:19:14]

Done.

+          ReasonForClose::UNKNOWN_ERROR);
+  } else if (packet.size() > GetConceptualMaxPacketSize()) {
+    Error("Can't receive packet greater than maximum packet size",
+          ReasonForClose::UNKNOWN_ERROR);
+  } else {
+    VerifyPacketCounter(packet);

[Kyle Horimoto, 2016/06/20 23:25:40]

If the packet counter is not verified correctly, you shouldn't continue.

[jingxuy, 2016/06/21 01:19:14]

this is caught on the case ERROR:

[Kyle Horimoto, 2016/06/21 02:10:26]

I realize that, but that case will cause "Received message in ERROR state." to
be printed. You never received a message in ERROR state, so this log will be
incorrect and could cause confusion to others while they are debugging. You
should probably have VerifyPacketCounter() return a boolean of whether the
verification succeeded or not and use that.

[Kyle Horimoto, 2016/06/21 18:13:37]

Ping.

[jingxuy, 2016/06/22 01:42:23]

Done.

+
+    switch (state_) {
+      case State::CONNECTING:
+        ReceiveFirstPacket(packet);
+        break;
+      case State::WAITING:
+        ReceiveNonFirstPacket(packet, true);
+        break;
+      case State::RECEIVING_DATA:
+        ReceiveNonFirstPacket(packet, false);
+        break;
+      case State::DATA_READY:
+        data_message_.clear();
+        ReceiveNonFirstPacket(packet, true);
+        break;
+      case State::CONNECTION_CLOSED:
+        Error("Receiving message in ConnectionClosed state.",
+              ReasonForClose::UNKNOWN_ERROR);
+        break;
+      case State::ERROR:
+        PA_LOG(ERROR) << "Receiving message in ERROR state.";
+        break;
+      default:
+        // Receiving an message in connection close or error state is not valid.

[Kyle Horimoto, 2016/06/20 23:25:39]

This should just be NOTREACHED();

We shouldn't ever get to this point.

[jingxuy, 2016/06/21 01:19:14]

Done.

+        Error("Receiving message in invalid state.",
+              ReasonForClose::UNKNOWN_ERROR);
+        break;
+    }
+  }
+  return state_;
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::ReceiveFirstPacket(
+    const Packet& packet) {
+  DCHECK(!packet.empty());
+  DCHECK(state_ == State::CONNECTING);
+
+  if (GetPacketType(packet) != PacketType::CONTROL) {
+    Error("Can't receive data packets when not connected.",
+          ReasonForClose::UNKNOWN_ERROR);
+    return;
+  }
+
+  uint8_t command = GetControlCommand(packet);
+  switch (command) {
+    case ControlCommand::CONNECTION_REQUEST:
+      if (receiver_type_ == ReceiverType::SERVER) {
+        ReceiveConnectionRequest(packet);
+      } else {
+        Error("Server couldn't process connection response.",
+              ReasonForClose::UNKNOWN_ERROR);
+      }
+      break;
+    case ControlCommand::CONNECTION_RESPONSE:
+      if (receiver_type_ == ReceiverType::CLIENT) {
+        ReceiveConnectionResponse(packet);
+      } else {
+        Error("Client couldn't process connection request.",
+              ReasonForClose::UNKNOWN_ERROR);
+      }
+      break;
+    case ControlCommand::CONNECTION_CLOSE:
+      Error("Shouldn't receive close without a connection.",
+            ReasonForClose::UNKNOWN_ERROR);
+      break;
+    default:
+      Error("Unrecognized control packet command.",
+            ReasonForClose::UNKNOWN_ERROR);
+      break;
+  }
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::ReceiveNonFirstPacket(
+    const Packet& packet,
+    bool expect_first_packet) {

[Kyle Horimoto, 2016/06/20 23:25:39]

There should be no need to pass this parameter. We can already derive this value
based on state_.

[jingxuy, 2016/06/21 01:19:14]

I was trying to avoid switching on the state twice. I think the switch(state)
should have divided the action by state. I didn't want the helper function to
also switch on state again.

[Kyle Horimoto, 2016/06/21 02:10:26]

I'm not sure what you mean. You can just use:

bool expect_first_packet = state_ == State::RECEIVING_DATA;

Make sure that you use that after the DCHECK() lines to ensure that you have
eliminated other possible error conditions by that point.

[jingxuy, 2016/06/21 06:19:26]

It's more of a code style issue. I was trying to test all the state problems
within the switch(state) in ReceivePacket. This function is intended to be not
state cognisant. Instead of checking the states here, the state is checked in
the switch statement and passed in here.

[Kyle Horimoto, 2016/06/21 18:13:37]

In general, you should strive to make functions encapsulate as much logic as you
can so that clients of those functions do not have to know about the internals
of those functions in order to call them. Of course, this isn't always possible,
but it definitely is here.

Regarding your comment about this function not being "state cognizant," it IS
state cognizant:
1) The DCHECK() at the top checks that you are in the right state.
2) You call ReceiveConnectionClose(), which requires that you are in a specific
state to call it.
3) You call AppendData(), which requires that you are in a specific state to
call it.
4) You modify state_ internally.

So, since your DCHECK() essentially just checks that the expect_first_packet
value you passed in was correct, I think it makes more sense to just compute it
internally.

[jingxuy, 2016/06/22 01:42:23]

I don't really think DCHECK count as part of the logic really. And I don't
really think it's good practice to switch on state twice in a state machine. But
I will change the implementation.

+  DCHECK(!packet.empty());
+  DCHECK(((state_ == State::WAITING || state_ == State::DATA_READY) &&
+          expect_first_packet) ||
+         (state_ == State::RECEIVING_DATA && !expect_first_packet));
+
+  switch (GetPacketType(packet)) {
+    case PacketType::CONTROL:
+      if (GetControlCommand(packet) == ControlCommand::CONNECTION_CLOSE) {
+        ReceiveConnectionClose(packet);
+      } else {
+        Error("Can only receive connection close during data transaction",
+              ReasonForClose::UNKNOWN_ERROR);
+      }
+      break;
+    case PacketType::DATA:
+      if (!IsLowerTwoBitsCleared(packet)) {
+        Error("Lower two bits of data packet header are not clear.",
+              ReasonForClose::UNKNOWN_ERROR);
+      } else if (expect_first_packet ^ IsFirstDataPacket(packet)) {

[Kyle Horimoto, 2016/06/20 23:25:39]

Why xor? If we expect that this is the first packet and it is the first packet,
then we would be XORing true and true, which would produce false. You should use
!= instead.

[jingxuy, 2016/06/21 01:19:14]

I think true and true are suppose to be false for that statement since it's not
an errorneous state because expect and isfirst are the same.
but oxr is actually equivalent to one != other, so I'll change it. it was oxr
because it made more sense in code I modified.

+        // This means that expectation of whether a packet would be a
+        // first packet and what we actually got are different.
+        Error("First bit of data packet is not set correctly.",
+              ReasonForClose::RECEIVED_PACKET_OUT_OF_SEQUENCE);
+      } else {
+        AppendData(packet, 1);
+        if (IsLastDataPacket(packet)) {
+          state_ = State::DATA_READY;
+        } else {
+          state_ = State::RECEIVING_DATA;
+        }
+      }
+      break;
+    default:
+      Error("Invalid packet type.", ReasonForClose::UNKNOWN_ERROR);
+      break;
+  }
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::ReceiveConnectionRequest(
+    const Packet& packet) {
+  DCHECK(!packet.empty());
+  DCHECK(state_ == State::CONNECTING);
+
+  if (packet.size() < kMinConnectionRequestSize ||
+      packet.size() > kMaxControlPacketSize) {
+    Error("Invalid connection request packet size.",
+          ReasonForClose::UNKNOWN_ERROR);
+    return;
+  }
+
+  uint16_t packet_size = GetShortField(packet, 5);
+  // Packet size of 0 means the server can observe the ATT_MTU and selecte an

[Kyle Horimoto, 2016/06/20 23:25:39]

Please use a constant instead of using 0 directly.

[Kyle Horimoto, 2016/06/20 23:25:39]

nit: s/selecte/select/

[jingxuy, 2016/06/21 01:19:14]

Done.

+  // appropriate packet size;
+  if (packet_size > 0 && packet_size < kMinPacketSize) {
+    Error("Client must support at least 20 bytes per packet.",
+          ReasonForClose::UNKNOWN_ERROR);
+    return;
+  }
+  SetMaxPacketSize(packet_size);
+
+  uint16_t min_version = GetShortField(packet, 1);
+  uint16_t max_version = GetShortField(packet, 3);
+  if (kSupportedWeaveVersion < min_version ||
+      kSupportedWeaveVersion > max_version) {
+    Error("Server does not support client version range.",
+          ReasonForClose::NO_COMMON_VERSION_SUPPORTED);
+    return;
+  }
+
+  if (packet.size() > kMinConnectionRequestSize) {
+    AppendData(packet, kMinConnectionRequestSize);
+    state_ = State::DATA_READY;
+  } else {
+    state_ = State::WAITING;
+  }
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::ReceiveConnectionResponse(
+    const Packet& packet) {
+  DCHECK(!packet.empty());
+  DCHECK(state_ == State::CONNECTING);
+
+  if (packet.size() < kMinConnectionResponseSize ||
+      packet.size() > kMaxControlPacketSize) {
+    Error("Invalid connection response packet size.",
+          ReasonForClose::UNKNOWN_ERROR);
+    return;
+  }
+
+  uint16_t selected_packet_size = GetShortField(packet, 3);
+  if (selected_packet_size < kMinPacketSize) {
+    Error("Server must support at least 20 bytes per packet.",

[Kyle Horimoto, 2016/06/20 23:25:39]

nit: Don't hard-code 20 in your error message. Use kMinPacketSize.

[jingxuy, 2016/06/21 01:19:14]

Done.

[Kyle Horimoto, 2016/06/21 02:10:26]

Use a std::stringstream instead of this.

[Kyle Horimoto, 2016/06/21 18:13:37]

Ping.

[jingxuy, 2016/06/22 01:42:23]

Done.

+          ReasonForClose::UNKNOWN_ERROR);
+    return;
+  }
+  SetMaxPacketSize(selected_packet_size);
+
+  uint16_t selected_version = GetShortField(packet, 1);
+  if (selected_version != kSupportedWeaveVersion) {
+    Error("Client does not support server selected version.",
+          ReasonForClose::NO_COMMON_VERSION_SUPPORTED);
+    return;
+  }
+
+  if (packet.size() > kMinConnectionResponseSize) {
+    AppendData(packet, kMinConnectionResponseSize);
+    state_ = State::DATA_READY;
+  } else {
+    state_ = State::WAITING;
+  }
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::ReceiveConnectionClose(
+    const Packet& packet) {
+  DCHECK(!packet.empty());
+
+  if (packet.size() > kMaxControlPacketSize) {
+    Error("Invalid connection close packet size.",
+          ReasonForClose::UNKNOWN_ERROR);
+    return;
+  }
+
+  if (packet.size() < kMinConnectionCloseSize) {
+    reason_for_close_ = ReasonForClose::UNKNOWN_ERROR;
+    return;
+  }
+
+  uint16_t reason = GetShortField(packet, 1);
+
+  switch (reason) {
+    case ReasonForClose::CLOSE_WITHOUT_ERROR:
+    case ReasonForClose::UNKNOWN_ERROR:
+    case ReasonForClose::NO_COMMON_VERSION_SUPPORTED:
+    case ReasonForClose::RECEIVED_PACKET_OUT_OF_SEQUENCE:
+    case ReasonForClose::APPLICATION_ERROR:
+      reason_for_close_ = static_cast<ReasonForClose>(reason);
+      state_ = State::CONNECTION_CLOSED;
+      break;
+    default:
+      Error("Invalid reason for close.", ReasonForClose::UNKNOWN_ERROR);
+      break;
+  }
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::AppendData(const Packet& packet,
+                                                       uint32_t byte_offset) {
+  DCHECK(!packet.empty());
+
+  // Append to data_message_ bytes 1 through end of the packet.
+  data_message_.insert(data_message_.end(), packet.begin() + byte_offset,
+                       packet.end());
+}
+
+uint16_t BluetoothLowEnergyWeavePacketReceiver::GetShortField(
+    const Packet& packet,
+    uint32_t byte_offset) {
+  DCHECK_LT(byte_offset, packet.size());
+  DCHECK_LT(byte_offset + 1, packet.size());
+
+  // packet[byte_offset + 1] is the upper byte and packet[byte_offset] is the

[Kyle Horimoto, 2016/06/20 23:25:39]

As I mentioned in https://codereview.chromium.org/2031953003/, please use
ntohs() and/or htons(). We can't assume endianness.

[jingxuy, 2016/06/21 01:19:14]

I think Gustavo already replied and resolved this. The endianness of the packet
is guaranteed to be network endiannness and I am returning an uint16 by shifting
and not writing to memory directly.

[Kyle Horimoto, 2016/06/21 02:10:26]

You are correct that the packet is guaranteed to be in network endianness, but
you cannot assume that your host endianness is the same as the network
endianness. If the Chromebook running this code has the opposite endianness, the
wrong value will be produced.

[jingxuy, 2016/06/21 06:19:26]

I was referring to Gustavo's comment on writing or not writing to memory
directly.
The endianness only matter if you are addressing memory directly.
For instance you have uint16_t val and have uint8_t* val_ptr = (uint8_t
*)(&uint16_t);
then you write to val_ptr[0] and val_ptr[1]. This is accessing bytes in memory.
However if you have uint16_t val = upper << 8 | lower, this is not writing to
memory,
it's performing a mathematical calculation which would be upper * 2^8 + lower,
which
is guaranteed to be correct or otherwise a computer breaks down.

[Kyle Horimoto, 2016/06/21 18:13:37]

I think the misunderstanding is that the direct 2-byte value that we receive is
not necessarily encoded in the same way that your machine encodes things.

It might be helpful to use an example. Pretend instead that instead of
endianness, we are dealing with encrypting/decrypting a value. A 2-byte value is
encrypted in the network, and we have received it. Now, we need to decrypt it
into a value that our computer understands. To do so, we would have to run some
special decrypt() function that takes in the network-encrypted 2-byte value and
returns the decrypted 2-byte value in order for your machine to use it the right
way.

Now, suppose that the encryption technique used is that you just swap the two
bytes, so encrypt(0xff00) returns 0x00ff, and decrypt(0x00ff) returns 0xff00
again. This is exactly what happens in this endianness situation. However, you
don't necessarily know if the endianness on the network is the same as on the
machine, so on machines with the same endianness as the network htons() and
ntohs() just return the same value that they were passed. That's why you can't
assume endianness and must use these functions.

[jingxuy, 2016/06/22 01:42:23]

This is resolved in the generator as a correct implementation but I'll implement
the ntohs version.

+  // lower byte.
+  return (packet[byte_offset + 1] << 8) | packet[byte_offset];
+}
+
+uint8_t BluetoothLowEnergyWeavePacketReceiver::GetPacketType(
+    const Packet& packet) {
+  DCHECK(!packet.empty());
+  // Packet type is stored in the highest bit of the first byte.
+  return (packet[0] >> 7) & 1;
+}
+
+uint8_t BluetoothLowEnergyWeavePacketReceiver::GetControlCommand(
+    const Packet& packet) {
+  DCHECK(!packet.empty());
+  // Control command is stored in the lower 4 bits of the first byte.
+  return packet[0] & 0x0F;
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::VerifyPacketCounter(
+    const Packet& packet) {
+  if (state_ == State::ERROR)
+    return;
+
+  DCHECK(!packet.empty());
+  // Packet counter is bits 4, 5, and 6 of the first byte.
+  uint8_t count = (packet[0] >> 4) & 7;
+
+  if (count == (packet_number_ % kMaxPacketCounter)) {
+    packet_number_++;
+  } else {
+    Error("Invalid packet counter.",
+          ReasonForClose::RECEIVED_PACKET_OUT_OF_SEQUENCE);
+  }
+}
+
+bool BluetoothLowEnergyWeavePacketReceiver::IsFirstDataPacket(
+    const Packet& packet) {
+  DCHECK(!packet.empty());
+  // Bit 3 determines whether the packet is the first packet of the message.
+  return (packet[0] >> 3) & 1;
+}
+
+bool BluetoothLowEnergyWeavePacketReceiver::IsLastDataPacket(
+    const Packet& packet) {
+  DCHECK(!packet.empty());
+  // Bit 2 determines whether the packet is the last packet of the message.
+  return (packet[0] >> 2) & 1;
+}
+
+bool BluetoothLowEnergyWeavePacketReceiver::IsLowerTwoBitsCleared(
+    const Packet& packet) {
+  DCHECK(!packet.empty());
+  return (packet[0] & 3) == 0;
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::Error(
+    std::string error_message,
+    ReasonForClose reason_to_close) {
+  PA_LOG(ERROR) << error_message;
+  state_ = State::ERROR;
+  reason_to_close_ = reason_to_close;
+}
+
+void BluetoothLowEnergyWeavePacketReceiver::SetMaxPacketSize(
+    uint16_t packet_size) {
+  DCHECK(packet_size == 0 || packet_size >= kMinPacketSize);
+  max_packet_size_ = packet_size;
+}
+
+uint16_t BluetoothLowEnergyWeavePacketReceiver::GetConceptualMaxPacketSize() {
+  if (!max_packet_size_)
+    return 20;
+  return max_packet_size_;
+}
+
+}  // namespace proximity_auth