Fix NMT ndims overflow crash

[GustavoA1604]

What problem does this PR solve?

nmt_model_load read the per-tensor n_dims straight from the GGML weight header and used it as the loop bound into a fixed 4-int stack array with no range check:

• A model file with n_dims > 4 writes past int32_t ne[4], corrupting the stack and crashing the native process (STATUS_ACCESS_VIOLATION / 0xC0000005) on load.
• Reachable through the public TranslationNmtcpp.load() API for any untrusted or unverified model (user/URL-supplied, or a registry/CDN fetch with no hash or signature). Loading, not translating, is the trigger.
• The same loop also allocated std::vector<char> tmp(length) from an unchecked file-controlled length and never verified the name read returned the requested bytes.

How does it solve it?

• Extract the dimension-reading loop into nmt_read_tensor_dims, which rejects n_dims outside [1, 4] before any read.
• Bound the tensor-name length before allocation, and fail the load unless the name read returns the full requested byte count.
• Introduce NMT_MAX_TENSOR_DIMS and NMT_MAX_TENSOR_NAME_LENGTH constants instead of magic numbers.
• Add addon/tests/nmt_loader_test.cpp: valid 2-D and 4-D headers succeed; the crafted n_dims=8 case and n_dims ∈ {0, -1} are rejected with zero bytes consumed and ne[] left intact.

Loading a legitimate model is unaffected (longest real tensor name is ~49 chars, well under the 256 bound).

Breaking changes

None.

[github-actions]

Review Status

Current Status: ❌ PENDING
Approvals so far: Team Lead: 1

Pending reviews: Needs 1 more from Management, Team Lead, or Member.

[github-actions]

License compliance — clean

No new dependency license findings in this PR.

Warn-only (shadow) mode — this check does not block merges yet.

Updated automatically by the canonical license compliance workflow.

NOTICE presence (advisory)

Missing NOTICE (advisory, does not block):

• ./docs/website
• ./packages/fabric/test/integration
• ./packages/llm-llamacpp/benchmarks/server
• ./packages/llm-llamacpp/benchmarks/performance
• ./packages/inference-addon-cpp/mobile
• ./packages/asr-ggml/benchmarks/server
• ./packages/embed-llamacpp/benchmarks/server
• ./packages/embed-llamacpp/benchmarks/performance
• ./packages/sdk/e2e
• ./packages/vla-ggml/sim/server
• ./.github/actions/release-merge-guard

[gianni-cor]

Summary

PR #4590 hardened nmt_model_load against a malformed tensor n_dims (stack overflow) and bounded the tensor name length. Reviewing the rest of the same function shows the fix is incomplete: several other values are still read straight from an untrusted model file and used without validation. All are reachable through the same public TranslationNmtcpp.load() API that #4590 hardens — loading (not translating) is the trigger, for any user/URL/registry-supplied model with no hash or signature check.

All findings are in packages/translation-nmtcpp/addon/src/model-interface/nmt_loader.cpp (nmt_model_load and its helpers load_vocab / load_sentencepiece_model).

1. Unvalidated ttype → out-of-bounds read (primary follow-up)

The tensor-header loop reads ttype from the file and passes it straight to ggml:

read_safe(loader, ttype);                       // file-controlled int32
...
const size_t bpe = ggml_type_size(ggml_type(ttype));   // no range check

ggml_type_size only guards its input with assert(type < GGML_TYPE_COUNT), which is compiled out in the Release build (-DNDEBUG). An out-of-range ttype therefore indexes type_traits[] out of bounds — an OOB read in the same untrusted-load path #4590 is hardening. Lower severity than the n_dims write it fixes (read vs. write), but the same threat model.

Fix: reject ttype < 0 || ttype >= GGML_TYPE_COUNT before use.

2. Unbounded, unchecked length reads (same class as the fixed tensor-name bug)

#4590 introduced nmtReadTensorName, which bounds a file-controlled length to NMT_MAX_TENSOR_NAME_LENGTH and fails unless the read returns the full byte count. Three other sites in the same function still use the exact unsafe pattern that helper was written to replace — a file-controlled length drives an allocation, the read() return value is ignored, and the string is then built from the requested size rather than the bytes actually read (so a truncated file yields a string over uninitialized heap):

• Source vocab tokens (load_vocab):

  uint32_t len; read_safe(loader, len);          // up to 4 GiB
  tmp.resize(len);
  loader->read(loader->context, &tmp[0], tmp.size());   // return ignored
  word.assign(&tmp[0], tmp.size());              // requested size, not bytes read

• Target vocab tokens (IndicTrans branch): identical pattern with token_len.
• SentencePiece blob (load_sentencepiece_model):

  int32_t sp_model_size; read_safe(loader, sp_model_size);   // up to 2 GiB
  std::vector<char> sp_model_data(sp_model_size);
  loader->read(loader->context, sp_model_data.data(), sp_model_size);  // return ignored
  std::string serialized_model(sp_model_data.data(), sp_model_size);   // fed to protobuf

Consequences: large-allocation denial of service, and uninitialized-heap contents assigned into strings / handed to the protobuf parser on a short read.

Fix: route all three through the same bound-and-verify helper as nmtReadTensorName (or a shared equivalent) — cap the length and require the read to return the full count.

3. Unvalidated loop counts

n_vocab and tgt_encoder_size are read from the file and used directly as loop bounds with no upper bound. Combined with #2, each iteration allocates from a file-controlled length, so a small file can drive very large work/allocation. (The vocab containers are std::map, so the loop index and token_id are safe as keys — no OOB there; the concern is unbounded iteration/allocation.)

Fix: sanity-bound the counts before looping.

Note on hparams

The hparams block (n_vocab, d_model, n_*_layers, *_ffn_dim, …) is also read wholesale from the file and later drives tensor-dimension math and reserve()/resize() calls. Bounding these is a larger change and out of scope here, but flagging it as the root of the same trust-boundary issue.

Verified

• Reproduced the pre-fix n_dims overflow under AddressSanitizer (stack-buffer-overflow, 4-byte write past int32_t ne[4]) and confirmed Fix NMT ndims overflow crash #4590's guard rejects it.
• Full addon-test suite (96 tests) passes on the PR head, including the new loader tests and the real 122M IndicTrans model load.
• Findings 1–3 verified by reading nmt_loader.cpp at the PR head (235061c). Containers confirmed std::map; the state-restore path has no untrusted deserialization.