Validate .nam model config fields instead of unchecked operator[] - #311
Open
constk wants to merge 1 commit into
Open
Validate .nam model config fields instead of unchecked operator[]#311constk wants to merge 1 commit into
constk wants to merge 1 commit into
Conversation
.nam files are untrusted input: hosts download and open them on behalf of users. The config parsers read required fields with `config["key"]`, which is nlohmann's const operator[]. That does JSON_ASSERT(key exists) then dereferences. JSON_ASSERT is plain assert (json.hpp:2571), so under NDEBUG -- i.e. any release plugin build -- a model file missing a required key dereferences a past-the-end iterator instead of throwing. Add nam::util helpers (NAM/json_util.h) that look fields up, report the offending key and its enclosing context, and throw std::runtime_error -- matching the "bad model file" convention already used throughout the loader. Route the required-field reads in the envelope (get_dsp), the WaveNet config parser, the Linear parser, and the activation config through them. Also validate values that previously flowed unchecked into allocations and arithmetic: - Dimensions (channels, input_size, condition_size, receptive_field, kernel sizes) are bounded to [1, 65536]. Previously a negative value became a huge size_t in resize() and any int was accepted. - groups fields must be >= 1. `groups_input: 0` reached `x % groups` and divided by zero. - Array lengths (layers, dilations, kernel_sizes) are bounded, so a small file cannot request millions of Layer objects. - Non-integral numbers are rejected rather than silently truncated. Fields that were optional stay optional, and no accepted value changes meaning, so any model that loads today still loads identically. Only inputs that were previously undefined behaviour now produce an error. Adds tools/test/test_model_validation.cpp covering each rejection plus positive tests that a well-formed WaveNet and Linear model still load. This does not fix the truncated-weights over-read (set_weights_ walks the weight vector with unchecked *(it++)); that needs a bounds-checked reader and is left for a follow-up.
Owner
|
Thanks! This seems useful. I added a bit of validation earlier today for #314 / motivated by sdatkinson/NeuralAmpModelerPlugin#569. The line diff on this is too large for me to get through at the moment but it is interesting to me. What I'd appreciate:
If done as I'm thinking, the diffs would be a one-liner call to your new validation functions at the starts of the If you can address these, happy to review. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
.namfiles are untrusted input — hosts download them and open them on a user's behalf. The config parsers read required fields withconfig["key"], which resolves to nlohmann's constoperator[]. That doesJSON_ASSERT(key exists)and then dereferences the iterator:JSON_ASSERTis plainassert(json.hpp:2571). UnderNDEBUG— i.e. any release plugin build — the assert vanishes and a model file missing a required key dereferences a past-the-end iterator instead of throwing. That's undefined behaviour reachable from a downloaded file.Alongside it, several attacker-controlled values reached allocations and arithmetic unchecked:
intand passed toEigen::resize. A negative value becomes a hugesize_t.groupswas never validated, and reachesif (in_channels % groups != 0)indsp.cpp:314/conv1d.cpp:60,65."groups_input": 0is a one-field divide-by-zero.Layerobjects."channels": 4.9→4).Approach
New
nam::utilhelpers inNAM/json_util.hlook a field up, name both the offending key and its enclosing context in the message, and throwstd::runtime_error— matching the "bad model file" convention already used throughout the loader (get_dsp.cpp:121,139,173,model_config.h:86, and ~25 sites inwavenet/model.cpp). They're kept deliberately small, and in their own header rather thanNAM/util.hso consumers don't pick up<Eigen/Dense>transitively.Required-field reads are routed through them in the envelope (
get_dsp), the WaveNet config parser, the Linear parser, and the activation config.Values are now bounded: dimensions to
[1, 65536],groups >= 1, and array lengths to 4096. Both caps are memory-safety bounds, not claims about which models are legitimate, and are commented as such.Compatibility
Any model that loads today still loads, identically. Fields that were optional (
.value(key, default)/.find()) stay optional — nothing was promoted to required — and no accepted value changes meaning. The only inputs whose behaviour changes are ones that were previously undefined behaviour or a crash.The
\throwsdocumentation added toget_dsp.hsaysstd::exceptionrather thanstd::runtime_error, because some malformed input still surfaces asnlohmann::json::exception, which derives fromstd::exceptionbut not fromstd::runtime_error. Documenting the narrower type would have been wrong and could send a host intostd::terminate().Tests
tools/test/test_model_validation.cppcovers each rejection — missingversion/architecture/config/layers, negative and oversized dimensions, empty and over-longdilations,"layer1x1": {}and"head1x1"missing inner keys,"activation": {},"groups_input": 0, FiLMgroups: 0, negativekernel_size, non-integralchannels, and the exactkMaxModelDimensionboundary — plus positive tests that a well-formed WaveNet and Linear model still load and process audio.Verified as follows:
-Werror, Debug, Release, andNAM_USE_INLINE_GEMM=ON;run_testsexits 0 on all three.clang-format19 reports every changed file clean.json.hpp:22188— the exactoperator[]assertion above. The tests genuinely catch the bug rather than passing incidentally.What this does not fix
Stated plainly so the scope isn't overread:
weightsarray is still a heap over-read.set_weights_walks the vector with unchecked*(it++)(conv1d.cpp:22,46,53,lstm.cpp:21-28,convnet.cpp:22-151), and the only bounds check runs after everything is consumed and detects surplus, not deficit. The dimension caps here do not mitigate it — a 1-layer, 1-channel model with an emptyweightsarray reproduces it just as easily. Fixing it properly means a bounds-checked weight reader, which re-typesset_weights_across several headers; that's a separate PR and I'd rather not bundle it.channelsat the 65536 cap still makes oneConv1Dtap request ~16 GiB. This closes the UB and the trivial crashes, not memory-exhaustion DoS.convnet.cppandlstm.cppstill use uncheckedoperator[]on required fields. Left out to keep this diff reviewable; happy to follow up.get_dsp(condition_dspatmodel.cpp, container submodels atcontainer.cpp:163) has no depth limit. Unchanged by this PR, but worth knowing about.Review notes
Reviewed locally by a code-review and a security pass before opening. Both initially returned request changes — the first version hardened the top-level fields but left the identical
JSON_ASSERTUB live in thelayer1x1/head1x1sub-objects and inactivations.cpp, which would have made for a misleadingly-named "hardening" change. Those are fixed here, along with thegroupsdivide-by-zero and the array-length bound, which the security pass surfaced.Happy to split this further, adjust the cap values, or drop the
json_utilhelper in favour of inline checks if you'd prefer a smaller surface.