get_dsp() aborts the process on well-formed JSON that is not a NAM model
Nobody has claimed this yet.
Assessment
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Newbie friendliness
- 86/100
Research direction
Start in NAM/get_dsp.cpp at populate_dsp_data and reproduce the issue with the provided well-formed JSON containing only hello. Ensure missing version, architecture, or config produces a recoverable exception rather than an assertion or process abort, while preserving the existing weights handling; verify the reproduction reaches the catch block.
Written by the indexing model from the issue text.
Description
Summary
nam::get_dsp() aborts the process when handed well-formed JSON that is not a
NAM model. It does not throw, so consumers cannot recover — the whole application
dies.
This is distinct from #314, which was about malformed JSON leaking
nlohmann::json::parse_error. Here the JSON parses fine; it just lacks the keys
Core expects.
It matters for any consumer that accepts files from the user. Ours loads whatever
gets dropped on the window, so a stray .json — a config file, a package.json,
anything — takes the application down with it.
Reproduction
#include <iostream>
#include "NAM/get_dsp.h"
#include "json.hpp"
int main() {
const nlohmann::json j = nlohmann::json::parse(R"({"hello": "world"})");
std::cout << "about to call get_dsp()\n" << std::flush;
try {
auto dsp = nam::get_dsp(j);
std::cout << "loaded (unexpected)\n";
} catch (const std::exception& e) {
std::cout << "threw, as one would hope: " << e.what() << "\n";
}
return 0;
}
Observed on macOS 15 with AppleClang, -O2:
about to call get_dsp()
Assertion failed: (it != m_data.m_value.object->end()), function operator[], file json.hpp, line 22188.
Abort trap: 6
Exit code 134. Adding -DNDEBUG to the get_dsp.cpp translation unit did not
change the outcome in my testing.
Reproduced on v0.5.4; the same unguarded accesses are on main (2563c0f).
Root cause
populate_dsp_data in NAM/get_dsp.cpp indexes three keys with operator[]
without checking that they exist:
verify_config_version(config["version"].get<std::string>());
nlohmann::json config_json = config["config"];
returnedConfig.version = config["version"].get<std::string>();
returnedConfig.architecture = config["architecture"].get<std::string>();
On a const nlohmann::json, operator[] with a missing key asserts, and where
assertions are compiled out it is undefined behaviour. Either way it cannot be
relied on to throw.
weights is handled correctly a few lines up — j.find("weights") followed by a
std::runtime_error — so the fix is to bring the other three in line.
Suggested fix
for (const char* key : {"version", "architecture", "config"}) {
if (!config.contains(key))
throw std::runtime_error(std::string("Corrupted model file is missing ") + key + ".");
}
Or config.at(key), which throws nlohmann::json::out_of_range rather than
asserting — though a std::runtime_error would match the surrounding style and
what consumers already catch.
A type check on version and architecture would help too: {"version": 7, ...}
reaches .get<std::string>() and throws a json::type_error, which is a
different exception type from the std::runtime_error the rest of the loading
path uses.
Workaround for consumers
Validate before calling in:
if (!config.is_object() ||
!config.contains("version") || !config["version"].is_string() ||
!config.contains("architecture") || !config["architecture"].is_string() ||
!config.contains("config")) {
return /* your own load error */;
}
- Dominant language
- C++
- Stars
- 931
- Forks
- 178
- Avg merge
- 3h 31m
- Merged PRs (30d)
- 7
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from sdatkinson/NeuralAmpModelerCore
-
documentation
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
sdatkinson/NeuralAmpModelerCore#305 · 1 comment ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 65/100
sdatkinson/NeuralAmpModelerCore#304 · 2 comments ·
-
Difficulty 4/5 3-5 days Newbie friendliness 55/100
sdatkinson/NeuralAmpModelerCore#327 · 1 reaction ·
-
Difficulty 4/5 3-5 days Newbie friendliness 45/100
sdatkinson/NeuralAmpModelerCore#303 · 2 comments · 1 reaction ·
-
breaking
Difficulty 5/5 Over a week Newbie friendliness 35/100
sdatkinson/NeuralAmpModelerCore#294 · 1 reaction ·
All issues in sdatkinson/NeuralAmpModelerCore
Similar issues
-
Difficulty 2/5 1-3 hours Newbie friendliness 86/100
-
Sensor initialization takes very long when `--initial-sim-time` is set to current UNIX timestamp Open
Difficulty 2/5 1-3 hours Newbie friendliness 78/100
gazebosim/gz-sensors#662 · 1 comment ·
-
enhancement
Difficulty 2/5 1-3 hours Newbie friendliness 76/100
-
comp-datalake
Difficulty 2/5 1-3 hours Newbie friendliness 88/100
ClickHouse/ClickHouse#121222 ·
-
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
LadybirdBrowser/ladybird#12123 ·