Skip to content

Improve safety guards on metapack header parsing #752

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

Merged
jviotti merged 4 commits into from
Mar 18, 2026
Merged

Improve safety guards on metapack header parsing #752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
821aa7d
Improve safety guards on metapack header parsing
jviotti Mar 18, 2026
291ed89
Nicer optionals
jviotti Mar 18, 2026
7d11183
Fixes
jviotti Mar 18, 2026
b83a43a
More
jviotti Mar 18, 2026
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
31 changes: 26 additions & 5 deletions src/index/explorer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <cmath> // std::lround
#include <cstring> // std::memcpy
#include <filesystem> // std::filesystem
#include <limits> // std::numeric_limits
#include <numeric> // std::accumulate
#include <optional> // std::optional
#include <regex> // std::regex, std::regex_search, std::smatch
Expand Down Expand Up @@ -222,6 +223,15 @@ static auto make_explorer_schema_extension(
const std::string_view dialect, const std::string_view title,
const std::string_view description, const std::string_view alert,
const std::string_view provenance) -> std::vector<std::uint8_t> {
assert(path.size() <= std::numeric_limits<std::uint16_t>::max());
assert(identifier.size() <= std::numeric_limits<std::uint16_t>::max());
assert(base_dialect.size() <= std::numeric_limits<std::uint16_t>::max());
assert(dialect.size() <= std::numeric_limits<std::uint16_t>::max());
assert(title.size() <= std::numeric_limits<std::uint16_t>::max());
assert(description.size() <= std::numeric_limits<std::uint16_t>::max());
assert(alert.size() <= std::numeric_limits<std::uint16_t>::max());
assert(provenance.size() <= std::numeric_limits<std::uint16_t>::max());

const auto strings_size{
path.size() + identifier.size() + base_dialect.size() + dialect.size() +
title.size() + description.size() + alert.size() + provenance.size()};
Expand Down Expand Up @@ -273,9 +283,13 @@ struct GENERATE_EXPLORER_SCHEMA_METADATA {
const auto &resolver_entry{resolver.entry(action.data)};
// Read the schema to get data and bytes
sourcemeta::core::FileView schema_view{action.dependencies.front()};
const auto schema_info{sourcemeta::one::metapack_info(schema_view)};
const auto schema_data{
const auto schema_info_option{sourcemeta::one::metapack_info(schema_view)};
assert(schema_info_option.has_value());
const auto &schema_info{schema_info_option.value()};
const auto schema_data_option{
sourcemeta::one::metapack_read_json(action.dependencies.front())};
assert(schema_data_option.has_value());
const auto &schema_data{schema_data_option.value()};
const auto id{sourcemeta::core::identify(
schema_data, [&callback, &resolver](const auto identifier) {
return resolver(identifier, callback);
Expand Down Expand Up @@ -337,12 +351,16 @@ struct GENERATE_EXPLORER_SCHEMA_METADATA {
result.assign("examples", std::move(examples_array));
}

const auto health{
const auto health_option{
sourcemeta::one::metapack_read_json(action.dependencies.at(1))};
assert(health_option.has_value());
const auto &health{health_option.value()};
result.assign("health", health.at("score"));

const auto schema_dependencies{
const auto schema_dependencies_option{
sourcemeta::one::metapack_read_json(action.dependencies.at(2))};
assert(schema_dependencies_option.has_value());
const auto &schema_dependencies{schema_dependencies_option.value()};
result.assign("dependencies",
sourcemeta::core::to_json(schema_dependencies.size()));

Expand Down Expand Up @@ -499,7 +517,10 @@ struct GENERATE_EXPLORER_DIRECTORY_LIST {
dependency.parent_path().parent_path().filename().string()};

if (filename == "directory.metapack") {
auto directory_json{sourcemeta::one::metapack_read_json(dependency)};
auto directory_json_option{
sourcemeta::one::metapack_read_json(dependency)};
assert(directory_json_option.has_value());
auto directory_json{std::move(directory_json_option.value())};
assert(directory_json.is_object());
assert(directory_json.defines("health"));
assert(directory_json.at("health").is_integer());
Expand Down
39 changes: 30 additions & 9 deletions src/index/generators.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <cstring> // std::memcpy
#include <filesystem> // std::filesystem
#include <fstream> // std::ofstream
#include <limits> // std::numeric_limits
#include <memory> // std::unique_ptr
#include <mutex> // std::mutex, std::lock_guard
#include <queue> // std::queue
Expand All @@ -47,6 +48,7 @@ struct MetapackDialectExtension {

static auto make_dialect_extension(const std::string_view dialect)
-> std::vector<std::uint8_t> {
assert(dialect.size() <= std::numeric_limits<std::uint16_t>::max());
Copy link

@cubic-dev-ai cubic-dev-ai bot Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P2: The new size check relies on assert, so release builds can still truncate dialect.size() to 16 bits and serialize a corrupted dialect length. Use a runtime check before the cast.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/index/generators.h, line 51:

<comment>The new size check relies on `assert`, so release builds can still truncate `dialect.size()` to 16 bits and serialize a corrupted dialect length. Use a runtime check before the cast.</comment>

<file context>
@@ -47,6 +48,7 @@ struct MetapackDialectExtension {
 
 static auto make_dialect_extension(const std::string_view dialect)
     -> std::vector<std::uint8_t> {
+  assert(dialect.size() <= std::numeric_limits<std::uint16_t>::max());
   std::vector<std::uint8_t> result;
   result.resize(sizeof(MetapackDialectExtension) + dialect.size());
</file context>
Fix with Cubic

std::vector<std::uint8_t> result;
result.resize(sizeof(MetapackDialectExtension) + dialect.size());
MetapackDialectExtension header{};
Expand Down Expand Up @@ -179,8 +181,10 @@ struct GENERATE_POINTER_POSITIONS {
const sourcemeta::one::Configuration &,
const sourcemeta::core::JSON &) -> bool {
const auto timestamp_start{std::chrono::steady_clock::now()};
const auto schema{
const auto schema_option{
sourcemeta::one::metapack_read_json(action.dependencies.front())};
assert(schema_option.has_value());
const auto &schema{schema_option.value()};
std::ostringstream schema_stream;
sourcemeta::core::prettify(schema, schema_stream);
sourcemeta::core::PointerPositionTracker tracker;
Expand All @@ -204,8 +208,10 @@ struct GENERATE_FRAME_LOCATIONS {
const sourcemeta::one::Configuration &,
const sourcemeta::core::JSON &) -> bool {
const auto timestamp_start{std::chrono::steady_clock::now()};
const auto contents{
const auto contents_option{
sourcemeta::one::metapack_read_json(action.dependencies.front())};
assert(contents_option.has_value());
const auto &contents{contents_option.value()};
std::ostringstream contents_stream;
sourcemeta::core::prettify(contents, contents_stream);
sourcemeta::core::PointerPositionTracker tracker;
Expand Down Expand Up @@ -235,8 +241,10 @@ struct GENERATE_DEPENDENCIES {
const sourcemeta::one::Configuration &,
const sourcemeta::core::JSON &) -> bool {
const auto timestamp_start{std::chrono::steady_clock::now()};
const auto contents{
const auto contents_option{
sourcemeta::one::metapack_read_json(action.dependencies.front())};
assert(contents_option.has_value());
const auto &contents{contents_option.value()};
auto result{sourcemeta::core::JSON::make_array()};
sourcemeta::core::dependencies(
contents, sourcemeta::core::schema_walker,
Expand Down Expand Up @@ -290,7 +298,10 @@ struct GENERATE_DEPENDENTS {
std::unordered_set<sourcemeta::core::JSON::String>>;
DirectMap direct;
for (const auto &dependency : action.dependencies) {
const auto contents{sourcemeta::one::metapack_read_json(dependency)};
const auto contents_option{
sourcemeta::one::metapack_read_json(dependency)};
assert(contents_option.has_value());
const auto &contents{contents_option.value()};
assert(contents.is_array());
for (const auto &entry : contents.as_array()) {
direct[entry.at("to").to_string()].emplace(
Expand Down Expand Up @@ -356,8 +367,10 @@ struct GENERATE_HEALTH {
const sourcemeta::one::Configuration &,
const sourcemeta::core::JSON &) -> bool {
const auto timestamp_start{std::chrono::steady_clock::now()};
const auto contents{
const auto contents_option{
sourcemeta::one::metapack_read_json(action.dependencies.front())};
assert(contents_option.has_value());
const auto &contents{contents_option.value()};
const auto &collection{*resolver.entry(action.data).collection};
auto &cache_entry{bundle_for(collection, resolver, callback)};
auto errors{sourcemeta::core::JSON::make_array()};
Expand Down Expand Up @@ -454,8 +467,10 @@ struct GENERATE_BUNDLE {
const sourcemeta::one::Configuration &,
const sourcemeta::core::JSON &) -> bool {
const auto timestamp_start{std::chrono::steady_clock::now()};
auto schema{
auto schema_option{
sourcemeta::one::metapack_read_json(action.dependencies.front())};
assert(schema_option.has_value());
auto schema{std::move(schema_option.value())};
sourcemeta::core::bundle(schema, sourcemeta::core::schema_walker,
[&callback, &resolver](const auto identifier) {
return resolver(identifier, callback);
Expand Down Expand Up @@ -489,8 +504,10 @@ struct GENERATE_EDITOR {
const sourcemeta::one::Configuration &,
const sourcemeta::core::JSON &) -> bool {
const auto timestamp_start{std::chrono::steady_clock::now()};
auto schema{
auto schema_option{
sourcemeta::one::metapack_read_json(action.dependencies.front())};
assert(schema_option.has_value());
auto schema{std::move(schema_option.value())};
sourcemeta::core::for_editor(schema, sourcemeta::core::schema_walker,
[&callback, &resolver](const auto identifier) {
return resolver(identifier, callback);
Expand Down Expand Up @@ -521,8 +538,10 @@ static auto generate_blaze_template(
const sourcemeta::one::BuildPlan::Action::Dependencies &dependencies,
const sourcemeta::blaze::Mode mode) -> void {
const auto timestamp_start{std::chrono::steady_clock::now()};
const auto contents{
const auto contents_option{
sourcemeta::one::metapack_read_json(dependencies.front())};
assert(contents_option.has_value());
const auto &contents{contents_option.value()};
sourcemeta::core::SchemaFrame frame{
sourcemeta::core::SchemaFrame::Mode::References};
frame.analyse(contents, sourcemeta::core::schema_walker,
Expand Down Expand Up @@ -574,8 +593,10 @@ struct GENERATE_STATS {
const sourcemeta::one::Configuration &,
const sourcemeta::core::JSON &) -> bool {
const auto timestamp_start{std::chrono::steady_clock::now()};
const auto schema{
const auto schema_option{
sourcemeta::one::metapack_read_json(action.dependencies.front())};
assert(schema_option.has_value());
const auto &schema{schema_option.value()};
std::map<sourcemeta::core::JSON::String,
std::map<sourcemeta::core::JSON::String, std::uint64_t>>
result;
Expand Down
5 changes: 4 additions & 1 deletion src/index/index.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,10 @@ static auto index_main(const std::string_view &program,
if (entry.is_regular_file() && entry.path().extension() == ".metapack") {
try {
sourcemeta::core::FileView file_view{entry.path()};
const auto file_info{sourcemeta::one::metapack_info(file_view)};
const auto file_info_option{
sourcemeta::one::metapack_info(file_view)};
assert(file_info_option.has_value());
Copy link

@cubic-dev-ai cubic-dev-ai bot Mar 18, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P2: Using assert for a runtime parse failure can abort the process and bypass this function's exception handling path. Replace it with an explicit exception check so failures are handled consistently.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At src/index/index.cc, line 433:

<comment>Using `assert` for a runtime parse failure can abort the process and bypass this function's exception handling path. Replace it with an explicit exception check so failures are handled consistently.</comment>

<file context>
@@ -428,8 +428,10 @@ static auto index_main(const std::string_view &program,
-              sourcemeta::one::metapack_info(file_view).value()};
+          const auto file_info_option{
+              sourcemeta::one::metapack_info(file_view)};
+          assert(file_info_option.has_value());
+          const auto &file_info{file_info_option.value()};
           durations.emplace_back(entry.path(), file_info.duration);
</file context>
Fix with Cubic

const auto &file_info{file_info_option.value()};
durations.emplace_back(entry.path(), file_info.duration);
} catch (...) {
std::cerr << "Could not profile file: " << entry.path() << "\n";
Expand Down
8 changes: 5 additions & 3 deletions src/metapack/include/sourcemeta/one/metapack.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <chrono> // std::chrono
#include <cstdint> // std::uint8_t, std::uint16_t, std::uint32_t, etc.
#include <filesystem> // std::filesystem::path
#include <optional> // std::optional
#include <span> // std::span
#include <string_view> // std::string_view
#include <vector> // std::vector
Expand Down Expand Up @@ -88,14 +89,15 @@ auto metapack_write_file(const std::filesystem::path &destination,

SOURCEMETA_ONE_METAPACK_EXPORT
auto metapack_read_json(const std::filesystem::path &path)
-> sourcemeta::core::JSON;
-> std::optional<sourcemeta::core::JSON>;

SOURCEMETA_ONE_METAPACK_EXPORT
auto metapack_info(const sourcemeta::core::FileView &view) -> MetapackInfo;
auto metapack_info(const sourcemeta::core::FileView &view)
-> std::optional<MetapackInfo>;

SOURCEMETA_ONE_METAPACK_EXPORT
auto metapack_payload_offset(const sourcemeta::core::FileView &view)
-> std::size_t;
-> std::optional<std::size_t>;

SOURCEMETA_ONE_METAPACK_EXPORT
auto metapack_extension_offset(const sourcemeta::core::FileView &view)
Expand Down
81 changes: 57 additions & 24 deletions src/metapack/metapack.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
#include <cassert> // assert
#include <cstring> // std::memcpy
#include <fstream> // std::ofstream
#include <optional> // std::optional, std::nullopt
#include <sstream> // std::ostringstream, std::stringstream
#include <stdexcept> // std::runtime_error
#include <string> // std::string
#include <string_view> // std::string_view

Expand Down Expand Up @@ -162,22 +162,29 @@ auto metapack_extension_offset(const sourcemeta::core::FileView &view)
}

const auto *header{view.as<MetapackHeader>()};
assert(header->magic == METAPACK_MAGIC);
assert(header->format_version == METAPACK_VERSION);
if (header->magic != METAPACK_MAGIC ||
header->format_version != METAPACK_VERSION) {
return 0;
}

const auto offset_of_extension_size{sizeof(MetapackHeader) +
header->mime_length};
if (offset_of_extension_size + sizeof(std::uint32_t) > view.size()) {
return 0;
}

const auto *extension_size_pointer{
view.as<std::uint32_t>(offset_of_extension_size)};
if (*extension_size_pointer == 0) {
const auto extension_size{*view.as<std::uint32_t>(offset_of_extension_size)};
if (extension_size == 0) {
return 0;
}

const auto extension_data_offset{offset_of_extension_size +
sizeof(std::uint32_t)};
if (extension_data_offset + extension_size > view.size()) {
return 0;
}

return offset_of_extension_size + sizeof(std::uint32_t);
return extension_data_offset;
}

auto metapack_extension_size(const sourcemeta::core::FileView &view)
Expand All @@ -187,7 +194,10 @@ auto metapack_extension_size(const sourcemeta::core::FileView &view)
}

const auto *header{view.as<MetapackHeader>()};
assert(header->magic == METAPACK_MAGIC);
if (header->magic != METAPACK_MAGIC ||
header->format_version != METAPACK_VERSION) {
return 0;
}

const auto offset_of_extension_size{sizeof(MetapackHeader) +
header->mime_length};
Expand All @@ -199,27 +209,27 @@ auto metapack_extension_size(const sourcemeta::core::FileView &view)
}

auto metapack_read_json(const std::filesystem::path &path)
-> sourcemeta::core::JSON {
-> std::optional<sourcemeta::core::JSON> {
sourcemeta::core::FileView view{path};
if (view.size() < sizeof(MetapackHeader) + sizeof(std::uint32_t)) {
throw std::runtime_error("Metapack file too small");
return std::nullopt;
}

const auto *header{view.as<MetapackHeader>()};
if (header->magic != METAPACK_MAGIC) {
throw std::runtime_error("Invalid metapack magic");
if (header->magic != METAPACK_MAGIC ||
header->format_version != METAPACK_VERSION) {
return std::nullopt;
}

if (header->format_version != METAPACK_VERSION) {
throw std::runtime_error("Unsupported metapack version");
auto payload_offset{sizeof(MetapackHeader) + header->mime_length};
if (payload_offset + sizeof(std::uint32_t) > view.size()) {
return std::nullopt;
}

auto payload_offset{sizeof(MetapackHeader) + header->mime_length};
const auto *extension_size{view.as<std::uint32_t>(payload_offset)};
payload_offset += sizeof(std::uint32_t) + *extension_size;

if (payload_offset > view.size()) {
throw std::runtime_error("Metapack header extends past file end");
return std::nullopt;
}

const auto payload_data_size{view.size() - payload_offset};
Expand All @@ -239,11 +249,21 @@ auto metapack_read_json(const std::filesystem::path &path)
return sourcemeta::core::parse_json(payload_string);
}

auto metapack_info(const sourcemeta::core::FileView &view) -> MetapackInfo {
assert(view.size() >= sizeof(MetapackHeader) + sizeof(std::uint32_t));
auto metapack_info(const sourcemeta::core::FileView &view)
-> std::optional<MetapackInfo> {
if (view.size() < sizeof(MetapackHeader) + sizeof(std::uint32_t)) {
return std::nullopt;
}

const auto *header{view.as<MetapackHeader>()};
assert(header->magic == METAPACK_MAGIC);
assert(header->format_version == METAPACK_VERSION);
if (header->magic != METAPACK_MAGIC ||
header->format_version != METAPACK_VERSION) {
return std::nullopt;
}

if (sizeof(MetapackHeader) + header->mime_length > view.size()) {
return std::nullopt;
}

std::string checksum_hex;
checksum_hex.reserve(64);
Expand All @@ -270,14 +290,27 @@ auto metapack_info(const sourcemeta::core::FileView &view) -> MetapackInfo {
}

auto metapack_payload_offset(const sourcemeta::core::FileView &view)
-> std::size_t {
assert(view.size() >= sizeof(MetapackHeader) + sizeof(std::uint32_t));
-> std::optional<std::size_t> {
if (view.size() < sizeof(MetapackHeader) + sizeof(std::uint32_t)) {
return std::nullopt;
}

const auto *header{view.as<MetapackHeader>()};
assert(header->magic == METAPACK_MAGIC);
if (header->magic != METAPACK_MAGIC ||
header->format_version != METAPACK_VERSION) {
return std::nullopt;
}

auto offset{sizeof(MetapackHeader) + header->mime_length};
if (offset + sizeof(std::uint32_t) > view.size()) {
return std::nullopt;
}

const auto *extension_size{view.as<std::uint32_t>(offset)};
offset += sizeof(std::uint32_t) + *extension_size;
if (offset > view.size()) {
return std::nullopt;
}

return offset;
}
Expand Down
4 changes: 3 additions & 1 deletion src/resolver/resolver.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,10 @@ auto Resolver::operator()(
if (result->second.cache_path.has_value()) {
// We can guarantee the cached outcome is JSON, so we don't need to try
// reading as YAML
auto schema{
auto schema_option{
sourcemeta::one::metapack_read_json(result->second.cache_path.value())};
assert(schema_option.has_value());
auto schema{std::move(schema_option.value())};
assert(sourcemeta::core::is_schema(schema));
if (callback) {
callback(result->second.cache_path.value());
Expand Down
Loading
Loading