[Converter:Bugfix] Fix MNNConvert crash on malformed tflite models (#4796) - #4823
[Converter:Bugfix] Fix MNNConvert crash on malformed tflite models (#4796)#4823fshuang8299 wants to merge 1 commit into
Conversation
ff29790 to
88abfc7
Compare
…libaba#4796) MNNConvert -f TFLITE crashed with SIGSEGV on eight malformed models reported in alibaba#4796. All share the same root cause: values taken straight from the model file are used without a check that stops execution. Root causes fixed: - AsXxxOptions() returns nullptr when the builtin_options union tag does not match; the result was dereferenced unconditionally (Conv2D, DepthwiseConv2D, TransposeConv, Pool2D, FullyConnected). - DCHECK only logs and continues (logkit CHECK has no abort), so a weight shape that is not 4-D, non-positive dims, or a product that wraps in int fell through into element loads/writes. - Bias buffers were copied with memcpy / vector range-construction without verifying the buffer holds sizeof(T)*count bytes (float, INT8 and UINT8 paths of Conv2D and DepthwiseConv2D). - convertDataFormatTflite's DCHECKs were no-ops; it now validates dims and src/dst and returns false instead of writing through null. - ConcatSizeComputer dereferenced inputs[0] with an empty input list and trusted a rank that can overflow the 9-slot dim array. Fix: reject the malformed operator with the converter's idiomatic dstOp->type = OpType_MAX (liteConverter.cpp turns that into a clean conversion failure), validate weight shape/size with int64 arithmetic, bound-check every bias buffer, and make convertDataFormatTflite and ConcatSizeComputer fail closed. Notes on policy choices: - Depthwise INT8 weight with an empty/undersized buffer rejects the op, unlike Conv2D INT8 which skips the weight: the depthwise path assumes a constant weight buffer and cannot represent the op otherwise. - ConcatSizeComputer now rejects rank-0 inputs (the old code's dim[-1]/dim[axis] access on a scalar input was itself out of bounds). Verified: all 8 reproducers exit cleanly (8/8 SEGV on master), a legal conv+depthwise+pool+fc+concat model converts identically to baseline (only the random model UUID differs), and the full set is clean under AddressSanitizer.
88abfc7 to
9c35b5b
Compare
Verification ReportComplete verification of the fix against the 8 reproducers from #4796, plus regression checks. All tests run locally on x86_64 Linux. 1. Crash reproducers (functional)All 8 files attached to #4796, run with
2. No-regression: legal model conversionGenerated a legal FLOAT32 tflite model (CONV_2D → DEPTHWISE_CONV_2D → AVERAGE_POOL_2D → CONCATENATION, 8 tensors). Converted with master baseline and with this PR:
3. Quantized path (UINT8)Generated a UINT8-quantized CONV_2D model (with INT32 bias + quantization params). Conversion succeeds identically on baseline and this PR. 4. Memory safety (AddressSanitizer)ASAN build ( 5. MNN test suite
6. Style checks (matching repo CI)
Notes on policy choices
|
Summary
MNNConvert -f TFLITEsegfaults on eight malformed models reported in #4796 (all reproducers + ASAN traces attached there). Every crash is a value taken straight from the model file and used without a check that stops execution.Root causes
builtin_optionsunion:AsConv2DOptions()/AsPool2DOptions()/AsDepthwiseConv2DOptions()/AsTransposeConvOptions()/AsFullyConnectedOptions()returnnullptrwhen the union tag does not match, and the result was dereferenced unconditionally (e.g.fused_activation_functionread through null → SEGV at offset 0xc/0x8/0x14).DCHECKis log-and-continue:logkit.hdefinesDCHECK(x)asCHECK(x)whose body only prints (no abort), soDCHECK(weightShape.size() == 4)fell through intoweightShape[0..3]element loads on empty shapes.co * kh * kw * ciinintwraps for adversarial dims (product of large-but-positive dims wraps to a small positive value), producing an undersized destination followed by an out-of-bounds write insideconvertDataFormatTflite(the WRITE SEGV at TfliteUtils.cpp:107).::memcpy(biasData.data(), biasDataPtr, sizeof(float) * co)reads from the zero page / OOB when the bias buffer is empty or undersized.ConcatSizeComputer: dereferencedinputs[0]on an empty input list (assert commented out) and trusted a rank that can overflow the 9-slot dim array.Fix
Reject the malformed operator using the converter's idiomatic
dstOp->type = MNN::OpType_MAXpattern (liteConverter.cpp:384turns it into a clean conversion failure with exit code 1), and fail closed everywhere:builtin_optionsaccessor (Conv2D, DepthwiseConv2D, TransposeConv, Pool2D, FullyConnected).weightSizecomputed inint64does not overflowint— before any allocation or division (also protects theinputShape[3] / cigroup calc).size() >= sizeof(T) * countbeforememcpy/ vector range-construction (float, INT8 and UINT8 paths).convertDataFormatTflitenow validates dims and src/dst and returnsfalseinstead of looping through null pointers (defense in depth for all 4 call sites).ConcatSizeComputerrejects empty inputs, invalid rank (>MNN_MAX_TENSOR_DIM), and out-of-range axis (per input).Verification
Fixes #4796