Unallocated per_channel_output_multiplier variable when network is UInt8

[dfauvarq]

In tensorflow/lite/micro/kernels/cmsis_nn/depthwise_conv.cc the variable per_channel_output_multiplier is only initialized (line 96) for Int8 network leading to a crash if the passed network is UInt8

I propose to return an error when the type is UInt8 as it is not supported

Here is a proposed fix, I let you do the actual fix as you may want to make it more general

@@ -97,10 +97,14 @@ TfLiteStatus Prepare(TfLiteContext* context, TfLiteNode* node) { reinterpret_cast<int32_t*>(context->AllocatePersistentBuffer( context, num_channels * sizeof(int32_t))); data->reference_op_data.per_channel_output_shift = reinterpret_cast<int32_t*>(context->AllocatePersistentBuffer( context, num_channels * sizeof(int32_t))); + } else if (input->type == kTfLiteUInt8) { + TF_LITE_KERNEL_LOG(context, "Type %s (%d) not supported.", + TfLiteTypeGetName(input->type), input->type); + return kTfLiteError; }

[dfauvarq]

Same problem exist in tensorflow/lite/micro/kernels/cmsis_nn/conv.cc

@@ -94,10 +94,14 @@ TfLiteStatus Prepare(TfLiteContext* context, TfLiteNode* node) {
     static_cast<int32_t*>(context->AllocatePersistentBuffer(
         context, num_channels * sizeof(int32_t)));
 data->reference_op_data.per_channel_output_shift =
     static_cast<int32_t*>(context->AllocatePersistentBuffer(
         context, num_channels * sizeof(int32_t)));
+ } else if (input->type == kTfLiteUInt8) {
+   TF_LITE_KERNEL_LOG(context, "Type %s (%d) not supported.",
+                                      TfLiteTypeGetName(input->type), input->type);
+  return kTfLiteError;
}

[github-actions[bot]]

"This issue is being marked as stale due to inactivity. Remove label or comment to prevent closure in 5 days."

[github-actions[bot]]

"This issue is being closed because it has been marked as stale for 5 days with no further activity."

[iabdalkader]

Hi, is there any fix for this issue ? I don't actually need to run the uint8 model, but it should not crash the library.

[mansnils]

Thanks for raising this, it seems the error has been going under the radar. The posted code is a bit outdated. However neither CMSIS-NN or TFLM reference kernels supports uint8 except for dequantize/quantize. So expected behavior for TFLM is to return an error if trying to run a uint8 model.

[iabdalkader]

Thank you for looking into this!

The posted code is a bit outdated.

Here's a more recent backtrace if it helps. This function is in tensorflow/lite/kernels/kernel_util.cc, but according to others above, this occurs in more than one place. Note how the per_channel_shift pointer is set to whatever random value was in the memory (i.e., uninitialized):

tflite::PopulateConvolutionQuantizationParams (context=0x5f94, input=0x804547ec, filter=0x80454820, bias=0x80454854, 
    output=0x80454888, activation=@0x81e860e0: kTfLiteActNone, multiplier=0x81e85d6c, shift=0x81e85d70, 
    output_activation_min=0x81e85d7c, output_activation_max=0x81e85d80, per_channel_multiplier=0xff00ff00, 
    per_channel_shift=0xff00ff00, num_channels=8) at tensorflow/lite/kernels/kernel_util.cc:222
222	  TF_LITE_ENSURE_EQ(context, input->quantization.type,
(gdb) n
224	  TF_LITE_ENSURE_EQ(context, filter->quantization.type,
(gdb) n
233	  const auto* affine_quantization =
(gdb) 
235	  TF_LITE_ENSURE(context, affine_quantization);
(gdb) 
236	  TF_LITE_ENSURE(context, affine_quantization->scale);
(gdb) 
237	  const bool is_per_channel = affine_quantization->scale->size > 1;
(gdb) n
238	  if (is_per_channel) {
(gdb) n
251	  const float input_scale = input->params.scale;
(gdb) 
252	  const float output_scale = output->params.scale;
(gdb) n
254	  for (int i = 0; i < num_channels; ++i) {
(gdb) n
257	    const float scale = is_per_channel ? filter_scales[i] : filter_scales[0];
(gdb) n
264	    QuantizeMultiplier(effective_output_scale, &significand, &channel_shift);
(gdb) n
265	    per_channel_multiplier[i] = significand;
(gdb) n
266	    per_channel_shift[i] = channel_shift;
(gdb) print per_channel_shift
$9 = (int32_t *) 0xff00ff00
(gdb) print *per_channel_shift
Cannot access memory at address 0xff00ff00
(gdb) n
472	    ldr   r0,=HardFault_Handler

[rascani]

@mansnils - We should probably add a TF_LITE_ENSURE statement within the Prepare functions for the input type being one of the supported types. It doesn't look like we're erroring out cleanly. I think this code block prevents the allocation for the per_channel_shift for input types other than INT8 or INT16. We should probably just change that to be "not float."

[mansnils]

Right yes, sorry for closing this off to early. I propose we align this block with the reference kernel equivalent block. Should probably also align the allocation calls as you mention with the reference kernel.

[ArmRyan]

#2630