Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
237 changes: 237 additions & 0 deletions include/sframe/result.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
#pragma once

#include <utility>
#include <variant>
#include <string>

#include <namespace.h>

namespace SFRAME_NAMESPACE {

// Error types to replace exceptions
enum class SFrameErrorType
{
none = 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this option (but still start numbering at 1) and the SFrameError::ok() method.

internal_error,
invalid_parameter_error,
buffer_too_small_error,
crypto_error,
unsupported_ciphersuite_error,
authentication_error,
invalid_key_usage_error,
};

class SFrameError
{
public:
SFrameError()
: type_(SFrameErrorType::none)
, message_()
{
}

explicit SFrameError(SFrameErrorType type)
: type_(type)
, message_()
{
}

SFrameError(SFrameErrorType type, std::string message)
: type_(type)
, message_(std::move(message))
{
}

// Copy constructor
SFrameError(const SFrameError& other)
: type_(SFrameErrorType::none)
, message_(other.message_)
{
type_ = other.type_;
}
Comment on lines +46 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be non-default? Similar question with several of the methods below.


// Copy assignment
SFrameError& operator=(const SFrameError& other)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not clear that this type needs to be assignable. If it causes conflict with the const char* note below, I would drop the operator=.

{
if (this != &other) {
type_ = other.type_;
message_ = other.message_;
}
return *this;
}

// Move constructor
SFrameError(SFrameError&& other) noexcept
: type_(other.type_)
, message_(std::move(other.message_))
{
}

// Move assignment
SFrameError& operator=(SFrameError&& other) noexcept
{
if (this != &other) {
type_ = other.type_;
message_ = std::move(other.message_);
}
return *this;
}

SFrameErrorType type() const { return type_; }

const char* message() const { return message_.c_str(); }

bool ok() const { return type_ == SFrameErrorType::none; }

private:
SFrameErrorType type_ = SFrameErrorType::none;
std::string message_;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't love having STL containers here. Since we have a fixed list of messages, let's just define const char* constants for them and refer to them. Especially since that's how we're exposing them anyway!

};

// Helper to convert SFrameError to appropriate exception type
void
throw_on_error(const SFrameError& error);
Comment on lines +92 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, I would have a method Result::unwrap that throws on error. This also works well with eliminating SFrameErrorType::none. And it will save you having to write branches on is_ok().


template<typename T>
class Result
{
public:
typedef T element_type;

static Result<T> ok(const T& value) { return Result<T>(value); }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need the <T> here, or could these just be Result?


static Result<T> ok(T&& value) { return Result<T>(std::move(value)); }

static Result<T> err(SFrameErrorType error, const std::string& message = "")
{
return Result<T>(SFrameError(error, message));
}

static Result<T> err(SFrameError&& error)
{
return Result<T>(std::move(error));
}

Result(SFrameError error)
: data_(std::move(error))
{
}

Result(const T& value)
: data_(value)
{
}

Result(T&& value)
: data_(std::move(value))
{
}
Comment on lines +115 to +128
Copy link
Contributor

Choose a reason for hiding this comment

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

These should probably be private.


Result(const Result& other) = delete;
Result& operator=(const Result& other) = delete;

Result(Result&& other) noexcept
: data_(std::move(other.data_))
{
}

Result& operator=(Result&& other) noexcept
{
data_ = std::move(other.data_);
return *this;
}
Comment on lines +133 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these not the same as the default?


template<typename U>
Result(Result<U>&& other)
: data_(std::move(other.data_))
{
}

template<typename U>
Result& operator=(Result<U>&& other)
{
data_ = std::move(other.data_);
return *this;
}

T value() { return std::move(std::get<T>(data_)); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I would follow Rust here and call this unwrap. Note that it has "throw if error" semantics anyway, because if it contains an error, std::get will throw bad_variant_access if is_err.


SFrameError error()
{
if (std::holds_alternative<SFrameError>(data_)) {
auto error = std::get<SFrameError>(data_);
return error;
}
return SFrameError(); // Default OK error
}

bool is_ok() const { return std::holds_alternative<T>(data_); }

bool is_err() const { return std::holds_alternative<SFrameError>(data_); }

private:
std::variant<T, SFrameError> data_;
Comment on lines +172 to +173
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor note: If T = SFrameError, I think this class will fail to compile and/or produce weird results (e.g., from is_ok / is_err). I don't think this is a case we need to accommodate, though.

};

// Specialization for Result<void>
template<>
class Result<void>
{
public:
typedef void element_type;

static Result<void> ok() { return Result<void>(); }

static Result<void> err(SFrameErrorType error,
const std::string& message = "")
{
return Result<void>(SFrameError(error, message));
}

static Result<void> err(SFrameError&& error)
{
return Result<void>(std::move(error));
}

Result()
: is_ok_(true)
, error_()
{
}

Result(SFrameError error)
: is_ok_(false)
, error_(std::move(error))
{
}

Result(const Result& other) = delete;
Result& operator=(const Result& other) = delete;

Result(Result&& other) noexcept
: is_ok_(other.is_ok_)
, error_(std::move(other.error_))
{
}

Result& operator=(Result&& other) noexcept
{
is_ok_ = other.is_ok_;
error_ = std::move(other.error_);
return *this;
}

void value() { /* void has no value to move */ }

SFrameError error() { return error_; }

bool is_ok() const { return is_ok_; }

bool is_err() const { return !is_ok_; }

private:
bool is_ok_;
SFrameError error_;
Comment on lines +233 to +234
Copy link
Contributor

Choose a reason for hiding this comment

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

I would replace these with std::optional<SFrameError>. Even less need for SFrameError::none.

};

} // namespace SFRAME_NAMESPACE
43 changes: 31 additions & 12 deletions src/header.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,19 @@ encode_uint(uint64_t val, output_bytes buffer)
}
}

static uint64_t
static Result<uint64_t>
decode_uint(input_bytes data)
{
if (!data.empty() && data[0] == 0) {
throw invalid_parameter_error("Integer is not minimally encoded");
return Result<uint64_t>::err(SFrameErrorType::invalid_parameter_error,
"Integer is not minimally encoded");
}

uint64_t val = 0;
for (size_t i = 0; i < data.size(); i++) {
val = (val << 8) + static_cast<uint64_t>(data[i]);
}
return val;
return Result<uint64_t>::ok(val);
}

struct ValueOrLength
Expand Down Expand Up @@ -77,17 +78,23 @@ struct ValueOrLength
return value_or_length + 1;
}

std::tuple<uint64_t, input_bytes> read(input_bytes data) const
Result<std::tuple<uint64_t, input_bytes>> read(input_bytes data) const
{
if (!is_length) {
// Nothing to read; value is already in config byte
return { value_or_length, data };
return Result<std::tuple<uint64_t, input_bytes>>::ok(
std::make_tuple(value_or_length, data));
}

const auto size = value_size();
const auto value = decode_uint(data.subspan(0, size));
auto value_result = decode_uint(data.subspan(0, size));
if (!value_result.is_ok()) {
return value_result.error();
}
const auto value = value_result.value();
const auto remaining = data.subspan(size);
return { value, remaining };
return Result<std::tuple<uint64_t, input_bytes>>::ok(
std::make_tuple(value, remaining));
}

private:
Expand Down Expand Up @@ -140,20 +147,32 @@ Header::Header(KeyID key_id_in, Counter counter_in)
encode_uint(counter, after_kid.subspan(0, cfg.ctr.value_size()));
}

Header
Result<Header>
Header::parse(input_bytes buffer)
{
if (buffer.size() < Header::min_size) {
throw buffer_too_small_error("Ciphertext too small to decode header");
return Result<Header>::err(SFrameErrorType::buffer_too_small_error,
"Ciphertext too small to decode header");
}

const auto cfg = ConfigByte{ buffer[0] };
const auto after_cfg = buffer.subspan(1);
const auto [key_id, after_kid] = cfg.kid.read(after_cfg);
const auto [counter, _] = cfg.ctr.read(after_kid);

auto read_result = cfg.kid.read(after_cfg);
if (!read_result.is_ok()) {
return read_result.error();
}
auto [key_id, after_kid] = read_result.value();

read_result = cfg.ctr.read(after_kid);
if (!read_result.is_ok()) {
return read_result.error();
}
auto [counter, _] = read_result.value();

const auto encoded = buffer.subspan(0, cfg.encoded_size());

return Header(key_id, counter, encoded);
return Result<Header>::ok(Header(key_id, counter, encoded));
}

Header::Header(KeyID key_id_in, Counter counter_in, input_bytes encoded_in)
Expand Down
3 changes: 2 additions & 1 deletion src/header.h
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include <sframe/result.h>
#include <sframe/sframe.h>

namespace SFRAME_NAMESPACE {
Expand All @@ -14,7 +15,7 @@ class Header
const Counter counter;

Header(KeyID key_id_in, Counter counter_in);
static Header parse(input_bytes buffer);
static Result<Header> parse(input_bytes buffer);

input_bytes encoded() const { return _encoded; }
size_t size() const { return _encoded.size(); }
Expand Down
29 changes: 29 additions & 0 deletions src/result.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
#include <sframe/result.h>
#include <sframe/sframe.h>

namespace SFRAME_NAMESPACE {

void
throw_on_error(const SFrameError& error)
{
switch (error.type()) {
case SFrameErrorType::none:
return;
case SFrameErrorType::buffer_too_small_error:
throw buffer_too_small_error(error.message());
case SFrameErrorType::invalid_parameter_error:
throw invalid_parameter_error(error.message());
case SFrameErrorType::crypto_error:
throw crypto_error();
case SFrameErrorType::unsupported_ciphersuite_error:
throw unsupported_ciphersuite_error();
case SFrameErrorType::authentication_error:
throw authentication_error();
case SFrameErrorType::invalid_key_usage_error:
throw invalid_key_usage_error(error.message());
default:
throw std::runtime_error(error.message());
}
}

} // namespace SFRAME_NAMESPACE
12 changes: 10 additions & 2 deletions src/sframe.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,11 @@ Context::unprotect(output_bytes plaintext,
input_bytes ciphertext,
input_bytes metadata)
{
const auto header = Header::parse(ciphertext);
auto header_result = Header::parse(ciphertext);
if (!header_result.is_ok()) {
throw_on_error(header_result.error());
}
const auto header = header_result.value();
const auto inner_ciphertext = ciphertext.subspan(header.size());
return Context::unprotect_inner(
header, plaintext, inner_ciphertext, metadata);
Expand Down Expand Up @@ -271,7 +275,11 @@ MLSContext::unprotect(output_bytes plaintext,
input_bytes ciphertext,
input_bytes metadata)
{
const auto header = Header::parse(ciphertext);
auto header_result = Header::parse(ciphertext);
if (!header_result.is_ok()) {
throw_on_error(header_result.error());
}
const auto header = header_result.value();
const auto inner_ciphertext = ciphertext.subspan(header.size());

ensure_key(header.key_id, KeyUsage::unprotect);
Expand Down
Loading
Loading