From ae0cc92754bad54aefc51d5e426251deab6323dc Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Tue, 29 Apr 2025 17:58:14 -0400 Subject: [PATCH 1/6] Enable selective use of the C++ allocator --- .github/workflows/build.yml | 13 ++++++---- CMakeLists.txt | 6 +++++ Makefile | 3 +++ include/sframe/map.h | 4 ++++ include/sframe/sframe.h | 19 ++++++++++----- include/sframe/vector.h | 48 +++++++++++++++++++++++++++++++++++++ 6 files changed, 83 insertions(+), 10 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 1eb8c60..23a5f4b 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -51,15 +51,20 @@ jobs: ${{ env.CMAKE_BUILD_DIR }}/vcpkg_installed key: ${{ runner.os }}-${{ hashFiles( '**/vcpkg.json' ) }} - - name: configure to use clang-tidy and sanitizers + - name: Build and run unit tests (with allocator) run: | cmake -B "${{ env.CMAKE_BUILD_DIR }}" -DCLANG_TIDY=ON -DTESTING=ON -DSANITIZERS=ON -DCMAKE_TOOLCHAIN_FILE="${{ matrix.vcpkg-cmake-file}}" . + cmake --build "${{ env.CMAKE_BUILD_DIR }}" --config Release + ctest --test-dir "${{ env.CMAKE_BUILD_DIR }}" - - name: build + - name: Clear build artifacts run: | - cmake --build "${{ env.CMAKE_BUILD_DIR }}" --config Release + rm -rf "${{ env.CMAKE_BUILD_DIR }}" - - name: Unit tests + - name: Build and run unit tests (without allocator) run: | + cmake -B "${{ env.CMAKE_BUILD_DIR }}" -DCLANG_TIDY=ON -DTESTING=ON -DSANITIZERS=ON -DNO_ALLOC=ON -DCMAKE_TOOLCHAIN_FILE="${{ matrix.vcpkg-cmake-file}}" . + cmake --build "${{ env.CMAKE_BUILD_DIR }}" --config Release ctest --test-dir "${{ env.CMAKE_BUILD_DIR }}" + diff --git a/CMakeLists.txt b/CMakeLists.txt index b7c2daa..a0ceea4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -8,6 +8,7 @@ project(sframe option(TESTING "Build tests" OFF) option(CLANG_TIDY "Perform linting with clang-tidy" OFF) option(SANITIZERS "Enable sanitizers" OFF) +option(NO_ALLOC "Build without needing an allocator" OFF) # Use -DCRYPTO=(OPENSSL_1_1 | OPENSSL_3) to configure crypto if(NOT DEFINED CRYPTO) @@ -49,6 +50,11 @@ if(CLANG_TIDY) endif() endif() +if(NO_ALLOC) + message(STATUS "Configuring no-allocator version") + add_definitions(-DNO_ALLOC) +endif() + ### ### Dependencies ### diff --git a/Makefile b/Makefile index 56dfb81..34bc1e9 100644 --- a/Makefile +++ b/Makefile @@ -22,6 +22,9 @@ ${BUILD_DIR}: CMakeLists.txt test/CMakeLists.txt dev: CMakeLists.txt test/CMakeLists.txt cmake -B${BUILD_DIR} -DCMAKE_BUILD_TYPE=Debug -DCLANG_TIDY=ON -DTESTING=ON -DSANITIZERS=ON . +dev-nostd: CMakeLists.txt test/CMakeLists.txt + cmake -B${BUILD_DIR} -DCMAKE_BUILD_TYPE=Debug -DCLANG_TIDY=ON -DTESTING=ON -DSANITIZERS=ON -DNO_ALLOC=ON . + ${TEST_BIN}: ${LIB} test/* cmake --build ${BUILD_DIR} --target sframe_test diff --git a/include/sframe/map.h b/include/sframe/map.h index fcba567..69962f9 100644 --- a/include/sframe/map.h +++ b/include/sframe/map.h @@ -2,6 +2,8 @@ #include +namespace sframe { + template class map : private vector>, N> { @@ -64,3 +66,5 @@ class map : private vector>, N> std::replace_if(this->begin(), this->end(), to_erase, std::nullopt); } }; + +} // namespace sframe diff --git a/include/sframe/sframe.h b/include/sframe/sframe.h index ec87aa8..05effce 100644 --- a/include/sframe/sframe.h +++ b/include/sframe/sframe.h @@ -6,6 +6,17 @@ #include #include +// These constants define the size of certain internal data structures if +// we are configured not to depend on dynamic allocations, i.e., if the NO_ALLOC +// flag is set. If you are using an allocator, you can ignore them. +#ifndef SFRAME_MAX_KEYS +#define SFRAME_MAX_KEYS 200 +#endif + +#ifndef SFRAME_EPOCH_BITS +#define SFRAME_EPOCH_BITS 4 +#endif + namespace sframe { struct crypto_error : std::runtime_error @@ -58,7 +69,6 @@ using owned_bytes = vector; using KeyID = uint64_t; using Counter = uint64_t; - class Header; enum struct KeyUsage @@ -106,10 +116,8 @@ class Context static constexpr size_t max_metadata_size = 512; protected: - static constexpr size_t max_keys = 200; - CipherSuite suite; - map keys; + map keys; output_bytes protect_inner(const Header& header, output_bytes ciphertext, @@ -185,8 +193,7 @@ class MLSContext : protected Context const size_t epoch_bits; const size_t epoch_mask; - // XXX(RLB) Make this an attribute of the class? - static constexpr size_t max_epochs = 16; + static constexpr size_t max_epochs = 1 << SFRAME_EPOCH_BITS; vector, max_epochs> epoch_cache; }; diff --git a/include/sframe/vector.h b/include/sframe/vector.h index 874793a..27388ec 100644 --- a/include/sframe/vector.h +++ b/include/sframe/vector.h @@ -2,6 +2,10 @@ #include +#ifdef NO_ALLOC + +namespace sframe { + template class vector { @@ -81,3 +85,47 @@ class vector operator gsl::span() const { return gsl::span(_data).first(_size); } operator gsl::span() { return gsl::span(_data).first(_size); } }; + +} // namespace sframe + +#else // ifdef NO_ALLOC + +#include + +namespace sframe { + +template +class vector : public std::vector +{ +private: + using parent = std::vector; + +public: + constexpr vector() + : parent(N) + {} + + constexpr vector(size_t size) + : parent(size) + {} + + constexpr vector(gsl::span content) + : parent(content.begin(), content.end()) + {} + + template + constexpr vector(const vector& content) + : parent(content) + {} + + void append(gsl::span content) + { + const auto start = this->size(); + this->resize(start + content.size()); + std::copy(content.begin(), content.end(), this->begin() + start); + } +}; + +} // namespace sframe + +#endif From 3e25b98d820b5fe1e9e1c0962dfe155a5bc06b1b Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Tue, 29 Apr 2025 18:15:59 -0400 Subject: [PATCH 2/6] Use std::map --- include/sframe/map.h | 34 ++++++++++++++++++++++++++++++++++ include/sframe/vector.h | 14 +++++++++----- 2 files changed, 43 insertions(+), 5 deletions(-) diff --git a/include/sframe/map.h b/include/sframe/map.h index 69962f9..80b4541 100644 --- a/include/sframe/map.h +++ b/include/sframe/map.h @@ -1,5 +1,7 @@ #pragma once +#ifdef NO_ALLOC + #include namespace sframe { @@ -68,3 +70,35 @@ class map : private vector>, N> }; } // namespace sframe + +#else // ifdef NO_ALLOC + +#include + +namespace sframe { + +template +class map : public std::map +{ +private: + using parent = std::map; + +public: + bool contains(const K& key) const { return this->count(key) > 0; } + + template + void erase_if_key(F&& f) + { + for (auto iter = this->begin(); iter != this->end();) { + if (f(iter->first)) { + iter = this->erase(iter); + } else { + ++iter; + } + } + } +}; + +} // namespace sframe + +#endif // def NO_ALLOC diff --git a/include/sframe/vector.h b/include/sframe/vector.h index 27388ec..197b211 100644 --- a/include/sframe/vector.h +++ b/include/sframe/vector.h @@ -103,20 +103,24 @@ class vector : public std::vector public: constexpr vector() : parent(N) - {} + { + } constexpr vector(size_t size) : parent(size) - {} + { + } constexpr vector(gsl::span content) : parent(content.begin(), content.end()) - {} + { + } template constexpr vector(const vector& content) : parent(content) - {} + { + } void append(gsl::span content) { @@ -128,4 +132,4 @@ class vector : public std::vector } // namespace sframe -#endif +#endif // def NO_ALLOC From b0af38f8eee20c392eedfa2d8a16bd1fb7967767 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Tue, 29 Apr 2025 18:28:58 -0400 Subject: [PATCH 3/6] Add cautionary notes about inheritance --- include/sframe/map.h | 7 +++++++ include/sframe/vector.h | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/include/sframe/map.h b/include/sframe/map.h index 80b4541..b404181 100644 --- a/include/sframe/map.h +++ b/include/sframe/map.h @@ -77,6 +77,13 @@ class map : private vector>, N> namespace sframe { +// NOTE: NOT RECOMMENDED FOR USE OUTSIDE THIS LIBRARY +// +// We have used public inheritance from std::map to simplify the interface +// here. This works fine for the use cases we have within this library. If you +// choose to use this map type outside this library, you MUST NOT store it as a +// std::map pointer or reference. This will cause memory leaks, because the +// destructor ~std::map is not virtual. template class map : public std::map { diff --git a/include/sframe/vector.h b/include/sframe/vector.h index 197b211..649d2c3 100644 --- a/include/sframe/vector.h +++ b/include/sframe/vector.h @@ -94,6 +94,13 @@ class vector namespace sframe { +// NOTE: NOT RECOMMENDED FOR USE OUTSIDE THIS LIBRARY +// +// We have used public inheritance from std::vector to simplify the interface +// here. This works fine for the use cases we have within this library. If you +// choose to use this vector type outside this library, you MUST NOT store it as +// a std::vector pointer or reference. This will cause memory leaks, because +// the destructor ~std::vector is not virtual. template class vector : public std::vector { From 3be91c3a083dd07750c7a1e443fe16dc78e846b5 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Mon, 5 May 2025 11:56:51 -0400 Subject: [PATCH 4/6] Fix errors found by CI --- include/sframe/sframe.h | 5 +++++ src/crypto.cpp | 3 --- src/crypto_openssl11.cpp | 13 ++++++------- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/include/sframe/sframe.h b/include/sframe/sframe.h index 05effce..ba38e2a 100644 --- a/include/sframe/sframe.h +++ b/include/sframe/sframe.h @@ -9,6 +9,11 @@ // These constants define the size of certain internal data structures if // we are configured not to depend on dynamic allocations, i.e., if the NO_ALLOC // flag is set. If you are using an allocator, you can ignore them. +// +// Note that these constants must be the same at the time when the library is +// built as at the time when it is used. If you are using a pre-built binary, +// you must make sure that these parameters have the same values as when the +// library was built. #ifndef SFRAME_MAX_KEYS #define SFRAME_MAX_KEYS 200 #endif diff --git a/src/crypto.cpp b/src/crypto.cpp index 777ea02..92f9ca0 100644 --- a/src/crypto.cpp +++ b/src/crypto.cpp @@ -1,9 +1,6 @@ #include "crypto.h" #include "header.h" -#include -#include - namespace sframe { size_t diff --git a/src/crypto_openssl11.cpp b/src/crypto_openssl11.cpp index 5218029..b6f650a 100644 --- a/src/crypto_openssl11.cpp +++ b/src/crypto_openssl11.cpp @@ -149,23 +149,22 @@ hkdf_expand(CipherSuite suite, input_bytes prk, input_bytes info, size_t size) auto block = owned_bytes(0); const auto block_size = cipher_digest_size(suite); - auto counter = uint8_t(0x01); + auto counter = owned_bytes<1>(); + counter[0] = 0x01; while (out.size() < size) { - // for (auto start = size_t(0); start < out.size(); start += block_size) { auto h = HMAC(suite, prk); h.write(block); h.write(info); - h.write(owned_bytes<1>{ counter }); + h.write(counter); - block.resize(block.capacity()); - const auto md = h.digest(block); - block.resize(md.size()); + block.resize(block_size); + h.digest(block); const auto remaining = size - out.size(); const auto to_write = (remaining < block_size) ? remaining : block_size; out.append(input_bytes(block).first(to_write)); - counter += 1; + counter[0] += 1; } return out; From b6661694c8695b128a38d96d19bafa89a3c51843 Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Mon, 5 May 2025 12:50:12 -0400 Subject: [PATCH 5/6] Try to clean up YAML --- .github/workflows/build.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index c55c910..506b1e5 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -59,8 +59,12 @@ jobs: key: ${{ runner.os }}-${{ hashFiles( '**/vcpkg.json' ) }} - name: configure to use clang-tidy and sanitizers - run: > - cmake -B "${{ env.CMAKE_BUILD_DIR }}" -DTESTING=ON -DCLANG_TIDY=ON -DSANITIZERS=ON -DCMAKE_TOOLCHAIN_FILE="${{ matrix.vcpkg-cmake-file}}" -DCRYPTO="${{ matrix.crypto }}" -DVCPKG_MANIFEST_DIR="alternatives/${{ matrix.crypto }}" -DNO_ALLOC="${{ matrix.no_alloc }}" + run: 'cmake -B "${{ env.CMAKE_BUILD_DIR }}" \ + -DTESTING=ON -DCLANG_TIDY=ON -DSANITIZERS=ON \ + -DCMAKE_TOOLCHAIN_FILE="${{ matrix.vcpkg-cmake-file}}" \ + -DCRYPTO="${{ matrix.crypto }}" \ + -DVCPKG_MANIFEST_DIR="alternatives/${{ matrix.crypto }}" \ + -DNO_ALLOC="${{ matrix.no_alloc }}"' - name: build run: | From df882854a303095acf4379346be02750a041002c Mon Sep 17 00:00:00 2001 From: Richard Barnes Date: Mon, 5 May 2025 12:55:39 -0400 Subject: [PATCH 6/6] Another attempt at YAML cleanup --- .github/workflows/build.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 506b1e5..d5abc07 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -59,12 +59,12 @@ jobs: key: ${{ runner.os }}-${{ hashFiles( '**/vcpkg.json' ) }} - name: configure to use clang-tidy and sanitizers - run: 'cmake -B "${{ env.CMAKE_BUILD_DIR }}" \ - -DTESTING=ON -DCLANG_TIDY=ON -DSANITIZERS=ON \ - -DCMAKE_TOOLCHAIN_FILE="${{ matrix.vcpkg-cmake-file}}" \ - -DCRYPTO="${{ matrix.crypto }}" \ - -DVCPKG_MANIFEST_DIR="alternatives/${{ matrix.crypto }}" \ - -DNO_ALLOC="${{ matrix.no_alloc }}"' + run: cmake -B "${{ env.CMAKE_BUILD_DIR }}" + -DTESTING=ON -DCLANG_TIDY=ON -DSANITIZERS=ON + -DCMAKE_TOOLCHAIN_FILE="${{ matrix.vcpkg-cmake-file}}" + -DCRYPTO="${{ matrix.crypto }}" + -DVCPKG_MANIFEST_DIR="alternatives/${{ matrix.crypto }}" + -DNO_ALLOC="${{ matrix.no_alloc }}" - name: build run: |