paramdict: reject negative array and string lengths

[professor-moody]

The array and string length fields in ParamDict are read from the model file as signed ints and used without a lower bound.

In load_param_bin the array branch does:

d->params[id].v.create(len);
float* ptr = d->params[id].v;
nread = dr.read(ptr, sizeof(float) * len);

With a negative len, Mat::create computes w * elemsize as a size_t, which wraps, so the aligned total also wraps and the allocation request comes out small enough to succeed. The result is a small block whose refcount pointer lands before the allocation, followed by a read of sizeof(float) * len bytes into it. Against a build with AddressSanitizer this shows up as a heap-buffer-overflow write of 4060 bytes into a 76 byte region, from an ordinary ncnn::Net::load_param_bin() call on a crafted .param.bin.

Three places need it:

• load_param, text array at line 297, len comes from dr.scan("%d") with no check
• load_param_bin, string at line 554, checks len > 255 but not len < 0. A negative length makes (len + 3) / 4 * 4 negative, which converts to a huge size_t and throws an uncaught length_error out of the loader
• load_param_bin, array at line 591, no check at all

This is the same shape as the id fix in 5a0288f, which changed id >= NCNN_MAX_PARAM_COUNT to id < 0 || id >= NCNN_MAX_PARAM_COUNT in both functions. The length fields in those same two functions kept an upper bound or no bound.

The patch rejects negative lengths at all three sites, and also checks that the allocation actually succeeded before reading into it, since a large positive length can fail to allocate and leave a null data pointer.

Added test_paramdict_7 covering the three cases. It fails on current main and passes with the fix. Full test suite is 176/176 either way.

[tencent-adm]

All committers have signed the CLA.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 975d4f056d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Encode binary fixtures as little-endian bytes

On big-endian builds, both added binary fixtures use the host-native representation of int, but load_param_bin() treats the stream as little-endian and byte-swaps each field under __BIG_ENDIAN__. The initial -23304 is therefore transformed into an unrelated out-of-range ID, so the loader returns an error before reaching the negative-length check; consequently, this test still passes if that check is removed. Use explicit little-endian byte arrays, as the earlier binary ParamDict tests do, so the intended branch is exercised on every supported endianness.

Useful? React with 👍 / 👎.

[professor-moody]

Good catch, thanks. Fixed in 5f13051.

Both binary fixtures now use explicit little-endian byte arrays, matching the existing binary ParamDict tests, instead of a host-native int array. You were right that the old version would have passed even with the fix removed on a big-endian build: the leading -23304 was byte-swapped into an out-of-range id, so load_param_bin returned before it ever reached the length check.

I also re-checked the three cases against an unpatched tree to confirm the test really does fail without the fix. The text case returns 0 where it should return -1, and both binary cases abort before completing.

[professor-moody]

Two corrections to the mechanism I described in the PR body. The patch itself is unchanged and I believe it is correct; these are both cases where a reviewer who tries the stated behaviour will see something different from what I wrote.

1. The length_error claim only holds for len <= -7. I wrote that a negative length makes (len + 3) / 4 * 4 negative, converts to a huge size_t, and throws. That is true for len <= -7. For len in [-6, -1] C++ integer division truncates toward zero, so the expression evaluates to 0: len_padded is 0, the buffer is 1 byte, dr.read reads 0 bytes, and the loader accepts the record as an empty string and returns 0. No throw, no diagnostic. So for those values the failure is silent acceptance of malformed input rather than a crash, which is arguably the worse case. A maintainer spot-checking with len = -1 sees success and could reasonably conclude the report was overstated.

2. The small-allocation explanation attributes the wrap to the wrong layer. I wrote that Mat::create computes w * elemsize as a size_t which wraps, so the aligned total also wraps and the request comes out small. The aligned total does not come out small: for len = -4 it stays at 0xFFFFFFFFFFFFFFF0, about 1.8e19. The request only becomes small one level deeper, inside fastMalloc, which adds its own overhead and carries past 2^64. Someone reading mat.cpp and computing totalsize will get a huge number and may conclude the report is wrong.

Two riders on that second point. The specific "76 byte region" in my ASan quote is branch dependent: it comes from the malloc fallback path, and a build taking the posix_memalign branch gives a 52 byte region and reports a different first error. And for len in [-1, -3] the aligned total wraps to 0 instead, so no allocation happens at all.

Neither correction changes the fix. Rejecting a negative length is right for all of these cases, including the silent-acceptance ones.