Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a buffer reader helper class that can be used to read buffers safely. #3471

Merged
merged 1 commit into from
Oct 27, 2020
Merged

Add a buffer reader helper class that can be used to read buffers safely. #3471

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/lib/support/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ static_library("support") {
"Base64.h",
"BitFlags.h",
"BufBound.h",
"BufferReader.h",
"CHIPArgParser.cpp",
"CHIPCounter.cpp",
"CHIPCounter.h",
Expand Down
150 changes: 150 additions & 0 deletions src/lib/support/BufferReader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,150 @@
/*
*
* Copyright (c) 2020 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

/**
* @file
* Utility classes for safely reading from size-limited buffers.
*/

#pragma once

#include <climits>
#include <lib/core/CHIPEncoding.h>
#include <lib/core/CHIPError.h>
#include <lib/support/CodeUtils.h>
#include <stdint.h>

namespace chip {
namespace Encoding {
namespace LittleEndian {

/**
* @class Reader
*
* Simple reader for reading little-endian things out of buffers.
*/
class Reader
{
public:
/**
* Create a data model reader from a given buffer and length.
*
* @param buffer The octet buffer to read from. The caller must ensure
* (most simply by allocating the reader on the stack) that
* the buffer outlives the reader. The buffer is allowed to
* be null if buf_len is 0.
* @param buf_len The number of octets in the buffer.
*/
Reader(const uint8_t * buffer, uint16_t buf_len) : mBufStart(buffer), mReadPtr(buffer), mAvailable(buf_len) {}

/**
* Number of octets we have read so far. This might be able to go away once
* we do less switching back and forth between DataModelReader and raw
* buffers.
*/
uint16_t OctetsRead() const { return static_cast<uint16_t>(mReadPtr - mBufStart); }

/**
* Read a single 8-bit unsigned integer.
*
* @param [out] dest Where the 8-bit integer goes.
*
* @return Whether the read succeeded. The read can fail if there are not
* enough octets available.
*/
CHECK_RETURN_VALUE
CHIP_ERROR Read8(uint8_t * dest) { return Read(dest); }

/**
* Read a single 16-bit unsigned integer.
*
* @param [out] dest Where the 16-bit integer goes.
*
* @return Whether the read succeeded. The read can fail if there are not
* enough octets available.
*/
CHECK_RETURN_VALUE
CHIP_ERROR Read16(uint16_t * dest) { return Read(dest); }

/**
* Read a single 32-bit unsigned integer.
*
* @param [out] dest Where the 32-bit integer goes.
*
* @return Whether the read succeeded. The read can fail if there are not
* enough octets available.
*/
CHECK_RETURN_VALUE
CHIP_ERROR Read32(uint32_t * dest) { return Read(dest); }

/**
* Read a single 64-bit unsigned integer.
*
* @param [out] dest Where the 64-bit integer goes.
*
* @return Whether the read succeeded. The read can fail if there are not
* enough octets available.
*/
CHECK_RETURN_VALUE
CHIP_ERROR Read64(uint64_t * dest) { return Read(dest); }

protected:
template <typename T>
CHIP_ERROR Read(T * retval)
{
static_assert(CHAR_BIT == 8, "Our various sizeof checks rely on bytes and octets being the same thing");

static constexpr size_t data_size = sizeof(T);

if (mAvailable < data_size)
Copy link
Contributor

@andy31415 andy31415 Oct 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought: should we somehow try to make errors persistent? If I have (wrong really) code like:

Reader reader(buffer, 2);

reader.Read32(&foo);
return reader.Read16(&bar);

we do not notice that a value failed to be read in between. I imagine we generally want to either use some GetLength() call to figure out available length (if data is variable size) or once reader failed to read, it became invalid.

If we have persistent errors, we can have builder patters:

Reader reader(buffer, len);

return reader.Read8(&foo).Read16(&bar).Read8(&baz).StatusCode();

instead of a more verbose VerifyOrExit:

CHIP_ERROR err = CHIP_NO_ERROR;
Reader reader(buffer, len);

err = reader.Read8(&foo);
SuccessOrExit(err);

err = reader.Read16(&bar);
SuccessOrExit(err);

err = reader.Read8(&baz);
SuccessOrExit(err);

exit:
   return err;

I could have similar SuccessOrExit(reader.StatusCode()) if I wanted to for some performance benefits, however I generally expect happy path to almost alwyas happen and would be willing to trade some ifs cost on failure for smaller flash usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I have (wrong really) code like

That code won't compile, because Read32 is marked as CHECK_RETURN_VALUE and in this code that return value is being ignored.

You could have code like this, though:

CHIP_RESULT res;
Reader reader(buffer, 2);

res = reader.Read32(&foo);
return reader.Read16(&bar);

but hopefully at that point you would notice that you are not doing anything with res. And even better would be if we could make the compiler notice...

If we have persistent errors, we can have builder patterns:

That does make sense, in cases when all the reads are unconditional. In cases when the reads are conditional on the value a previous read returned, people would need to make sure to examine the success value of the previous read. I guess we could maybe do that if we require that the reference we return will be used (for either an immediate unconditional read call or for an error check)?

One other note: I plan to build a data-model-specific reader on top of this one to address #2168. But I suspect that might also be possible with the reader pattern. Will check.

I'll prototype out this suggestion and see how the code and the resulting codesize look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy31415 OK, I put up some example code at https://github.com/bzbarsky-apple/connectedhomeip/tree/little-endian-reader-builder and https://github.com/bzbarsky-apple/connectedhomeip/tree/data-model-message-reader-builder for the builder pattern approach. It seems to use more codesize than the code I had before, though maybe it would do a bit better with some more out-of-lining, since Read ends up with a bit more logic.

Fundamentally, the builder setup still ends up doing the "did the previous call succeed" on every read check, which is what the SuccessOrExit was doing...

I did test instead setting mAvailable to 0 on read failure, so we don't have the extra branch on the status, but the end result is the same in terms of codesize; presumably the assignment is about the same size as the status-check branch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can iterate. I would expect it to be smaller in size, but only if used a lot (so that the save of no extra if is worth it to extra logic inside reads).

I am ok with the PR as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that works. I'm pretty happy to revisit once we have more consumers and can get better size data. Converting should not be that big a problem, I suspect....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some more experimenting, and I think you're right and the builder pattern might be a better fit here. Filed #3495 and will put up a PR soon.

{
return CHIP_ERROR_BUFFER_TOO_SMALL;
}

Read(mReadPtr, retval);
mAvailable = static_cast<uint16_t>(mAvailable - data_size);
return CHIP_NO_ERROR;
}

private:
// These helper methods return void and put the value being read into an
// outparam because that allows us to easily overload on the type of the
// thing being read.
void Read(const uint8_t *& p, uint8_t * dest) { *dest = Encoding::Read8(p); }
void Read(const uint8_t *& p, uint16_t * dest) { *dest = Encoding::LittleEndian::Read16(p); }
void Read(const uint8_t *& p, uint32_t * dest) { *dest = Encoding::LittleEndian::Read32(p); }
void Read(const uint8_t *& p, uint64_t * dest) { *dest = Encoding::LittleEndian::Read64(p); }

/**
* Our buffer start.
*/
const uint8_t * const mBufStart;

/**
* Our current read point.
*/
const uint8_t * mReadPtr;

/**
* The number of octets we can still read starting at mReadPtr.
*/
uint16_t mAvailable;
};

} // namespace LittleEndian
} // namespace Encoding
} // namespace chip
72 changes: 43 additions & 29 deletions src/transport/raw/MessageHeader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@

#include <type_traits>

#include <core/CHIPEncoding.h>
#include <core/CHIPError.h>
#include <support/BufferReader.h>
#include <support/CodeUtils.h>

/**********************************************
Expand Down Expand Up @@ -137,27 +137,29 @@ uint16_t MessageAuthenticationCode::TagLenForEncryptionType(Header::EncryptionTy

CHIP_ERROR PacketHeader::Decode(const uint8_t * const data, uint16_t size, uint16_t * decode_len)
{
CHIP_ERROR err = CHIP_NO_ERROR;
const uint8_t * p = data;
CHIP_ERROR err = CHIP_NO_ERROR;
LittleEndian::Reader reader(data, size);
int version;

VerifyOrExit(size >= kFixedUnencryptedHeaderSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT);
uint16_t octets_read;

uint16_t header;
header = LittleEndian::Read16(p);
err = reader.Read16(&header);
SuccessOrExit(err);
version = ((header & kVersionMask) >> kVersionShift);
VerifyOrExit(version == kHeaderVersion, err = CHIP_ERROR_VERSION_MISMATCH);

mEncryptionType = static_cast<Header::EncryptionType>((header & kEncryptionTypeMask) >> kEncryptionTypeShift);
mFlags.SetRaw(header & Header::kFlagsMask);

mMessageId = LittleEndian::Read32(p);
err = reader.Read32(&mMessageId);
SuccessOrExit(err);

if (mFlags.Has(Header::FlagValues::kSourceNodeIdPresent))
{
static_assert(kNodeIdSizeBytes == 8, "Read64 might read more bytes than we have in the buffer");
VerifyOrExit(size >= static_cast<size_t>(p - data) + kNodeIdSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT);
mSourceNodeId.SetValue(LittleEndian::Read64(p));
uint64_t sourceNodeId;
err = reader.Read64(&sourceNodeId);
SuccessOrExit(err);
mSourceNodeId.SetValue(sourceNodeId);
}
else
{
Expand All @@ -166,20 +168,25 @@ CHIP_ERROR PacketHeader::Decode(const uint8_t * const data, uint16_t size, uint1

if (mFlags.Has(Header::FlagValues::kDestinationNodeIdPresent))
{
VerifyOrExit(size >= static_cast<size_t>(p - data) + kNodeIdSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT);
mDestinationNodeId.SetValue(LittleEndian::Read64(p));
uint64_t destinationNodeId;
err = reader.Read64(&destinationNodeId);
SuccessOrExit(err);
mDestinationNodeId.SetValue(destinationNodeId);
}
else
{
mDestinationNodeId.ClearValue();
}

VerifyOrExit(size >= static_cast<size_t>(p - data) + sizeof(uint16_t) * 2, err = CHIP_ERROR_INVALID_ARGUMENT);
mEncryptionKeyID = LittleEndian::Read16(p);
mPayloadLength = LittleEndian::Read16(p);
err = reader.Read16(&mEncryptionKeyID);
SuccessOrExit(err);

VerifyOrExit(p - data == EncodeSizeBytes(), err = CHIP_ERROR_INTERNAL);
*decode_len = static_cast<uint16_t>(p - data);
err = reader.Read16(&mPayloadLength);
SuccessOrExit(err);

octets_read = reader.OctetsRead();
VerifyOrExit(octets_read == EncodeSizeBytes(), err = CHIP_ERROR_INTERNAL);
*decode_len = octets_read;

exit:

Expand All @@ -188,20 +195,26 @@ CHIP_ERROR PacketHeader::Decode(const uint8_t * const data, uint16_t size, uint1

CHIP_ERROR PayloadHeader::Decode(Header::Flags flags, const uint8_t * const data, uint16_t size, uint16_t * decode_len)
{
CHIP_ERROR err = CHIP_NO_ERROR;
const uint8_t * p = data;
CHIP_ERROR err = CHIP_NO_ERROR;
LittleEndian::Reader reader(data, size);
uint8_t header;
uint16_t octets_read;

err = reader.Read8(&header);
SuccessOrExit(err);

VerifyOrExit(size >= kEncryptedHeaderSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT);
err = reader.Read8(&mMessageType);
SuccessOrExit(err);

header = Read8(p);
mMessageType = Read8(p);
mExchangeID = LittleEndian::Read16(p);
err = reader.Read16(&mExchangeID);
SuccessOrExit(err);

if (flags.Has(Header::FlagValues::kVendorIdPresent))
{
VerifyOrExit(size >= static_cast<size_t>(p - data) + kVendorIdSizeBytes, err = CHIP_ERROR_INVALID_ARGUMENT);
mVendorId.SetValue(LittleEndian::Read16(p));
uint16_t vendor_id;
err = reader.Read16(&vendor_id);
SuccessOrExit(err);
mVendorId.SetValue(vendor_id);
}
else
{
Expand All @@ -210,11 +223,12 @@ CHIP_ERROR PayloadHeader::Decode(Header::Flags flags, const uint8_t * const data

mExchangeFlags.SetRaw(header);

VerifyOrExit(size >= static_cast<size_t>(p - data) + sizeof(uint16_t), err = CHIP_ERROR_INVALID_ARGUMENT);
mProtocolID = LittleEndian::Read16(p);
err = reader.Read16(&mProtocolID);
SuccessOrExit(err);

VerifyOrExit(p - data == EncodeSizeBytes(), err = CHIP_ERROR_INTERNAL);
*decode_len = static_cast<uint16_t>(p - data);
octets_read = reader.OctetsRead();
VerifyOrExit(octets_read == EncodeSizeBytes(), err = CHIP_ERROR_INTERNAL);
*decode_len = octets_read;

exit:

Expand Down