diff mbox series

[FFmpeg-devel] dnn_backend_native: Add overflow check for length calculation.

Message ID 20200706073217.3104-1-Reimar.Doeffinger@gmx.de
State Accepted
Headers show
Series [FFmpeg-devel] dnn_backend_native: Add overflow check for length calculation.
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

Reimar Döffinger July 6, 2020, 7:32 a.m. UTC
We should not silently allocate an incorrect sized buffer.
Fixes trac issue #8718.
TODO1: calculate_operand_dims_count is almost identical code,
should they be merged and its usages check for overflow?
TODO2: the -1 return value seems questionable to me, but is
aligned with the return value used for malloc failure.
Probably both ought to be changed to AVERROR(ENOMEM).

Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
---
 libavfilter/dnn/dnn_backend_native.c                   | 10 +++++++++-
 libavfilter/dnn/dnn_backend_native.h                   |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_conv2d.c      |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_depth2space.c |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_mathbinary.c  |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_mathunary.c   |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_maximum.c     |  2 ++
 libavfilter/dnn/dnn_backend_native_layer_pad.c         |  2 ++
 8 files changed, 23 insertions(+), 1 deletion(-)

--
2.27.0

Comments

Guo, Yejun July 6, 2020, 2:18 p.m. UTC | #1
> -----Original Message-----
> From: ffmpeg-devel <ffmpeg-devel-bounces@ffmpeg.org> On Behalf Of Reimar
> D?ffinger
> Sent: 2020年7月6日 15:32
> To: ffmpeg-devel@ffmpeg.org
> Subject: [FFmpeg-devel] [PATCH] dnn_backend_native: Add overflow check for
> length calculation.
> 
> We should not silently allocate an incorrect sized buffer.
> Fixes trac issue #8718.
> TODO1: calculate_operand_dims_count is almost identical code, should they be
> merged and its usages check for overflow?
> TODO2: the -1 return value seems questionable to me, but is aligned with the
> return value used for malloc failure.
> Probably both ought to be changed to AVERROR(ENOMEM).
> 
> Signed-off-by: Reimar Döffinger <Reimar.Doeffinger@gmx.de>
> ---
>  libavfilter/dnn/dnn_backend_native.c                   | 10 +++++++++-
>  libavfilter/dnn/dnn_backend_native.h                   |  2 ++
>  libavfilter/dnn/dnn_backend_native_layer_conv2d.c      |  2 ++
>  libavfilter/dnn/dnn_backend_native_layer_depth2space.c |  2 ++
> libavfilter/dnn/dnn_backend_native_layer_mathbinary.c  |  2 ++
>  libavfilter/dnn/dnn_backend_native_layer_mathunary.c   |  2 ++
>  libavfilter/dnn/dnn_backend_native_layer_maximum.c     |  2 ++
>  libavfilter/dnn/dnn_backend_native_layer_pad.c         |  2 ++
>  8 files changed, 23 insertions(+), 1 deletion(-)

thanks, LGTM, will push soon.

I'll modify a little to remove the TODO in commit log, and welcome the new patches.

I'll also cherry-pick to release 4.3
diff mbox series

Patch

diff --git a/libavfilter/dnn/dnn_backend_native.c b/libavfilter/dnn/dnn_backend_native.c
index 35236fc66f..a685efb092 100644
--- a/libavfilter/dnn/dnn_backend_native.c
+++ b/libavfilter/dnn/dnn_backend_native.c
@@ -79,6 +79,8 @@  static DNNReturnType set_input_output_native(void *model, DNNData *input, const

     av_freep(&oprd->data);
     oprd->length = calculate_operand_data_length(oprd);
+    if (oprd->length <= 0)
+        return DNN_ERROR;
     oprd->data = av_malloc(oprd->length);
     if (!oprd->data)
         return DNN_ERROR;
@@ -295,7 +297,13 @@  int32_t calculate_operand_dims_count(const DnnOperand *oprd)
 int32_t calculate_operand_data_length(const DnnOperand* oprd)
 {
     // currently, we just support DNN_FLOAT
-    return oprd->dims[0] * oprd->dims[1] * oprd->dims[2] * oprd->dims[3] * sizeof(float);
+    uint64_t len = sizeof(float);
+    for (int i = 0; i < 4; i++) {
+        len *= oprd->dims[i];
+        if (len > INT32_MAX)
+            return 0;
+    }
+    return len;
 }

 void ff_dnn_free_model_native(DNNModel **model)
diff --git a/libavfilter/dnn/dnn_backend_native.h b/libavfilter/dnn/dnn_backend_native.h
index bec63be450..62191ffe88 100644
--- a/libavfilter/dnn/dnn_backend_native.h
+++ b/libavfilter/dnn/dnn_backend_native.h
@@ -120,6 +120,8 @@  DNNReturnType ff_dnn_execute_model_native(const DNNModel *model, DNNData *output

 void ff_dnn_free_model_native(DNNModel **model);

+// NOTE: User must check for error (return value <= 0) to handle
+// case like integer overflow.
 int32_t calculate_operand_data_length(const DnnOperand *oprd);
 int32_t calculate_operand_dims_count(const DnnOperand *oprd);
 #endif
diff --git a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c
index c05bb5eca9..a2202e4073 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_conv2d.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_conv2d.c
@@ -113,6 +113,8 @@  int dnn_execute_layer_conv2d(DnnOperand *operands, const int32_t *input_operand_
     output_operand->dims[3] = conv_params->output_num;
     output_operand->data_type = operands[input_operand_index].data_type;
     output_operand->length = calculate_operand_data_length(output_operand);
+    if (output_operand->length <= 0)
+        return -1;
     output_operand->data = av_realloc(output_operand->data, output_operand->length);
     if (!output_operand->data)
         return -1;
diff --git a/libavfilter/dnn/dnn_backend_native_layer_depth2space.c b/libavfilter/dnn/dnn_backend_native_layer_depth2space.c
index 324871ceca..2c8bddf23d 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_depth2space.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_depth2space.c
@@ -75,6 +75,8 @@  int dnn_execute_layer_depth2space(DnnOperand *operands, const int32_t *input_ope
     output_operand->dims[3] = new_channels;
     output_operand->data_type = operands[input_operand_index].data_type;
     output_operand->length = calculate_operand_data_length(output_operand);
+    if (output_operand->length <= 0)
+        return -1;
     output_operand->data = av_realloc(output_operand->data, output_operand->length);
     if (!output_operand->data)
         return -1;
diff --git a/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c b/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c
index b239a20058..dd42c329a9 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_mathbinary.c
@@ -91,6 +91,8 @@  int dnn_execute_layer_math_binary(DnnOperand *operands, const int32_t *input_ope

     output->data_type = input->data_type;
     output->length = calculate_operand_data_length(output);
+    if (output->length <= 0)
+        return DNN_ERROR;
     output->data = av_realloc(output->data, output->length);
     if (!output->data)
         return DNN_ERROR;
diff --git a/libavfilter/dnn/dnn_backend_native_layer_mathunary.c b/libavfilter/dnn/dnn_backend_native_layer_mathunary.c
index 42615c43d5..8a4bd3c600 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_mathunary.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_mathunary.c
@@ -67,6 +67,8 @@  int dnn_execute_layer_math_unary(DnnOperand *operands, const int32_t *input_oper

     output->data_type = input->data_type;
     output->length = calculate_operand_data_length(output);
+    if (output->length <= 0)
+        return DNN_ERROR;
     output->data = av_realloc(output->data, output->length);
     if (!output->data)
         return DNN_ERROR;
diff --git a/libavfilter/dnn/dnn_backend_native_layer_maximum.c b/libavfilter/dnn/dnn_backend_native_layer_maximum.c
index af16e08b95..cdddfdd87b 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_maximum.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_maximum.c
@@ -64,6 +64,8 @@  int dnn_execute_layer_maximum(DnnOperand *operands, const int32_t *input_operand

     output->data_type = input->data_type;
     output->length = calculate_operand_data_length(output);
+    if (output->length <= 0)
+        return DNN_ERROR;
     output->data = av_realloc(output->data, output->length);
     if (!output->data)
         return DNN_ERROR;
diff --git a/libavfilter/dnn/dnn_backend_native_layer_pad.c b/libavfilter/dnn/dnn_backend_native_layer_pad.c
index dfbd204456..feaab001e8 100644
--- a/libavfilter/dnn/dnn_backend_native_layer_pad.c
+++ b/libavfilter/dnn/dnn_backend_native_layer_pad.c
@@ -111,6 +111,8 @@  int dnn_execute_layer_pad(DnnOperand *operands, const int32_t *input_operand_ind
     output_operand->dims[3] = new_channel;
     output_operand->data_type = operands[input_operand_index].data_type;
     output_operand->length = calculate_operand_data_length(output_operand);
+    if (output_operand->length <= 0)
+        return -1;
     output_operand->data = av_realloc(output_operand->data, output_operand->length);
     if (!output_operand->data)
         return -1;