| 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. | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
> -----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 --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;
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