fix: free Content object in error paths of loadModelFile/createFromBuffer#4384
fix: free Content object in error paths of loadModelFile/createFromBuffer#4384srpatcha wants to merge 7 commits into
Conversation
75d3f21 to
8c3a651
Compare
…ffer Both functions allocate Content with new but don't delete it when subsequent operations fail, leaking memory. Also fixed typo 'Memory not enought' -> 'Memory not enough'.
deepCopyQnnTensorInfo() passed unchecked malloc results to memcpy, causing null pointer dereference on allocation failure. strdup allocations were never freed on error paths. Added NULL checks after all allocations and proper cleanup on error paths. Signed-off-by: Srikanth Patchava <spatchava@meta.com> Signed-off-by: Srikanth Patchava <srikanth.patchava@outlook.com>
Add ImageOpTest.cpp with tests for: - Bilinear resize upscale/downscale with reference comparison - Nearest neighbor resize with exact pixel matching - Crop operations including boundary clamp edge cases - Rotation at 90, 180, 270 degrees - Colorspace: RGB↔BGR, RGB→GRAY, RGBA→GRAY - Edge cases: single pixel, large image (256x256), stride mismatch, identity transform - All tests registered with MNNTestSuiteRegister under cv/image_op/ Signed-off-by: Srikanth Patchava <spatchava@meta.com>
When the source coordinate is exactly at the boundary (e.g. x == xMax), ceilf(x) could return a value equal to the image dimension, causing out-of-bounds access in the source buffer. Fix by clamping y1 and x1 to ih-1 and iw-1 respectively, matching the clamp already applied to the float coordinates. Signed-off-by: Srikanth Patchava <spatchava@meta.com>
createFromBufferInternal takes ownership of the Content* parameter, but the early-return on line 124-126 (when checkNet fails) was missing 'delete net'. The two later error paths (lines 130 and 138) already free correctly. Same class of leak as the createFromBuffer/loadModelFile fix in the previous commit. Signed-off-by: Srikanth Patchava <spatchava@meta.com>
923a77d to
0dd70b2
Compare
|
Hi @srpatcha, friendly reminder — the core fixes (memory leak in Interpreter.cpp, bilinear boundary check, and QNN error handling) all look solid and correct. The only remaining item is consolidating the test file: please move the valuable new test cases (SinglePixelResizeTest, StrideMismatchTest) into the existing test/cv/ImageProcessTest.cpp and remove the separate ImageOpTest.cpp file to keep the test suite consolidated. Once that's done, this is ready to merge. Thanks for the contribution! |
There was a problem hiding this comment.
Please delete this file.
There was a problem hiding this comment.
We already have comprehensive image processing tests in test/cv/ImageProcessTest.cpp covering bilinear/nearest resize, colorspace conversions (RGB↔BGR, RGBA→Gray, BGR→Gray, etc.), YUV blitting, and transform-based resize.
Most of the tests in this new file overlap with existing ones. However, SinglePixelResizeTest and StrideMismatchTest are genuinely new edge-case scenarios that would be valuable additions.
Could you move just the useful new test cases (e.g., single-pixel resize, stride mismatch) into the existing test/cv/ImageProcessTest.cpp instead of creating a separate file? This keeps the test suite consolidated and avoids confusion between ImageProcessTest.cpp and ImageOpTest.cpp.
Move SinglePixelResizeTest and StrideMismatchTest from the separate ImageOpTest.cpp into the existing test/cv/ImageProcessTest.cpp test suite, and delete ImageOpTest.cpp. Test suite names updated from cv/image_op/* to cv/image_process/* to match the existing convention. Requested by @wangzhaode in review.
|
Hi @wangzhaode, done! Consolidated the test cases per your request:
Commit: daf87a3. Ready for final review! |
wangzhaode
left a comment
There was a problem hiding this comment.
Hi @srpatcha, thanks for the contribution! The core Interpreter.cpp memory leak fixes look solid and correct.
However, the PR currently fails to compile with -DMNN_BUILD_TEST=ON. The two new test cases introduced in test/cv/ImageProcessTest.cpp use unqualified enum names that are ambiguous in this translation unit:
test/cv/ImageProcessTest.cpp:1299:29: error: reference to 'NEAREST' is ambiguous
test/cv/ImageProcessTest.cpp:1337:29: error: reference to 'BILINEAR' is ambiguous
Both MNN::CV::Filter::BILINEAR and MNN::Express::BILINEAR are in scope, causing the ambiguity. The existing test cases in the same file all use fully qualified names.
Fix: please update the two lines to use the fully qualified enum:
// line 1299
config.filterType = MNN::CV::Filter::NEAREST;
// line 1337
config.filterType = MNN::CV::Filter::BILINEAR;Build command used:
cmake .. -DMNN_BUILD_LLM=ON -DMNN_LOW_MEMORY=ON -DMNN_BUILD_TEST=ON -DMNN_SUPPORT_TRANSFORMER_FUSE=ON -GNinja && ninjaPlease push the fix and we'll re-verify. Thanks!
Use MNN::CV::Filter::NEAREST and MNN::CV::Filter::BILINEAR in the consolidated test cases to avoid ambiguity between MNN::CV::Filter and MNN::Express enums when building with -DMNN_BUILD_TEST=ON. Fixes build error reported by @wangzhaode.
|
Hi @wangzhaode, fixed! Updated both lines to use fully qualified enum names:
Commit: a1b0b20. Should now compile cleanly with -DMNN_BUILD_TEST=ON. Thanks for catching this! |
|
Hi @srpatcha, thanks for this PR! The memory leak fixes in However, the two newly added tests ( The issue appears to be an incorrect call to the second overload of // Current (incorrect):
process->convert(src.data(), W, H, srcStride, dst.data(), W, H, channels * W, RGB);
// Signature:
// convert(source, iw, ih, stride, dest, ow, oh, outputBpp=0, outputStride=0, type=halide_type_of<float>())
Could you please:
The rest of the fixes (Interpreter leaks, bilinear clamp, QNN null checks) are solid — just need the test code corrected. Thanks! |
Fix memory leaks in model loading error paths and typo
Problem
Two memory leaks exist in
source/core/Interpreter.cpp:loadModelFile():new Contentis allocated but never freed ifloader->merge()fails.createFromBuffer():new Contentis allocated but never freed ifbuffer.reset()returns nullptr.Additionally, the error message "Memory not enought!" contains a typo.
Root Cause
Both functions allocate a
Contentobject withnewon the heap, then check a subsequent operation for failure. On failure, both returnnullptrwithout callingdeleteon the allocated object, leaking memory.Fix
delete net;beforereturn nullptr;in both error paths."Memory not enought!"→"Memory not enough!"Testing
loader->merge()failure path.Impact
Affects MNN users loading models, especially in long-running services or on memory-constrained devices where repeated failed model loads would accumulate leaked memory.