get_dsp() aborts the process on well-formed JSON that is not a NAM model

Open Beginner friendly
#328 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
86/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
cpp
Domain
backend

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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from sdatkinson/NeuralAmpModelerCore

All issues in sdatkinson/NeuralAmpModelerCore

Similar issues

More C++ issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.