From f93f519e3f3a0054d1a9d708c762ee707018d50e Mon Sep 17 00:00:00 2001 From: Seppo Ingalsuo Date: Wed, 29 Jul 2020 18:19:55 +0300 Subject: [PATCH 1/2] Audio: Fix pointer arithmetic mistake in ASRC initialize This patch fixes the issue where impulse response update can randomly corrupt stack in testbench environment. The error seen in freeing cd->asrc_obj was free(): invalid next size (normal). The violation was also confirmed with valgrind with error Invalid write of size 4. The order of impulse response storage and channel buffers are swapped to simplify the pointer initialize calculations. The default sizes for them are added as macro definitions into asrc_farrow.h. Checks for used sizes are added to buffer and filter initialize functions. The clear of buffer is done with memset instead of for loop. Fixes: 4c995d03753df9ed666f476a8d8ce6d4c28c3ff3 Signed-off-by: Seppo Ingalsuo --- src/audio/asrc/asrc_farrow.c | 150 +++++++++++------------ src/include/sof/audio/asrc/asrc_farrow.h | 10 +- 2 files changed, 81 insertions(+), 79 deletions(-) diff --git a/src/audio/asrc/asrc_farrow.c b/src/audio/asrc/asrc_farrow.c index 6dbe95f5c447..e93c187ebfe6 100644 --- a/src/audio/asrc/asrc_farrow.c +++ b/src/audio/asrc/asrc_farrow.c @@ -193,7 +193,8 @@ static const struct asrc_filter_params c_filter_params[CR_NUM] = { /* * Initialises the pointers to the buffers and zeroes their content */ -static enum asrc_error_code initialise_buffer(struct asrc_farrow *src_obj); +static enum asrc_error_code initialise_buffer(struct comp_dev *dev, + struct asrc_farrow *src_obj); /* * Initialise the pointers to the filters, set the number of filters @@ -212,15 +213,14 @@ static enum asrc_error_code initialise_filter(struct comp_dev *dev, * ------------------------------|-------------------------------------| * 0x0000 |asrc_farrow src_obj | * ------------------------------|-------------------------------------| - * &src_obj + sizeof(src_obj) |int_x *buffer_pointer[num_channels] | - * | | + * &src_obj + 1 |int32 impulse_response[filter_length]| + * ------------------------------|-------------------------------------| + * &impulse_response[0] + |int_x *buffer_pointer[num_channels] | + * filter_length | | * ------------------------------|-------------------------------------| - * &buffer_pointer[0] |int_x ring_buffer[num_channels | + * &buffer_pointer[0] + | int_x ring_buffer[num_channels * | * + num_channels*sizeof(int_x *)| *buffer_size] | - * ------------------------------|---------------------------------_---| - * &ring_buffer[0] |int32 impulse_response[filter_length]| - * + num_channels*buffer_size | | - * *sizeof(int_x) | | + * ------------------------------|-------------------------------------| * * Info: * @@ -237,8 +237,6 @@ enum asrc_error_code asrc_get_required_size(struct comp_dev *dev, int num_channels, int bit_depth) { - int filter_length = 128; - int buffer_length = 256; int size; /* check for parameter errors */ @@ -273,14 +271,17 @@ enum asrc_error_code asrc_get_required_size(struct comp_dev *dev, /* accumulate the size */ size = sizeof(struct asrc_farrow); - size += sizeof(int32_t *) * num_channels; /* pointers the the buffers */ - /* size of the ring buffers */ - size += buffer_length * num_channels * (bit_depth / 8); + /* size of the impulse response */ - size += filter_length * sizeof(int32_t); + size += ASRC_MAX_FILTER_LENGTH * sizeof(int32_t); - *required_size = size; + /* size of pointers to the buffers */ + size += sizeof(int32_t *) * num_channels; + + /* size of the ring buffers */ + size += ASRC_MAX_BUFFER_LENGTH * num_channels * (bit_depth / 8); + *required_size = size; return ASRC_EC_OK; } @@ -366,18 +367,10 @@ enum asrc_error_code asrc_initialise(struct comp_dev *dev, } /* - * The pointer to the internal ring buffer pointers is - * pointing to the memory subsequently to the memory where the - * src_obj lies. Only one of the buffers is initialised, - * depending on the specified bit depth. + * Set the pointer for the impulse response. It is just after + * src_obj in memory. */ - if (src_obj->bit_depth == 32) { - src_obj->ring_buffers32 = (int32_t **)(src_obj + 1); - src_obj->ring_buffers16 = NULL; - } else if (src_obj->bit_depth == 16) { - src_obj->ring_buffers16 = (int16_t **)(src_obj + 1); - src_obj->ring_buffers32 = NULL; - } + src_obj->impulse_response = (int32_t *)(src_obj + 1); /* * Load the filter coefficients and parameters. This function @@ -392,8 +385,23 @@ enum asrc_error_code asrc_initialise(struct comp_dev *dev, return error_code; } + /* + * The pointer to the internal ring buffer pointers is + * after impulse_response. Only one of the buffers is initialised, + * depending on the specified bit depth. + */ + if (src_obj->bit_depth == 32) { + src_obj->ring_buffers16 = NULL; + src_obj->ring_buffers32 = (int32_t **)(src_obj->impulse_response + + src_obj->filter_length); + } else if (src_obj->bit_depth == 16) { + src_obj->ring_buffers32 = NULL; + src_obj->ring_buffers16 = (int16_t **)(src_obj->impulse_response + + src_obj->filter_length); + } + /* set the channel pointers and fill buffers with zeros */ - error_code = initialise_buffer(src_obj); + error_code = initialise_buffer(dev, src_obj); /* check for errors */ if (error_code != ASRC_EC_OK) { @@ -401,26 +409,6 @@ enum asrc_error_code asrc_initialise(struct comp_dev *dev, return error_code; } - /* - * Set the pointer for the impulse response. - * &m_impulse_response[0] = ring_buffers_x - * + num_channels * (1 + buffer_length); - * Here ring_buffer_x already points to the memory just behind - * the src_obj. The offset results from the number of pointers - * pointing to each channel and each channels buffer data. - */ - if (src_obj->bit_depth == 32) { - src_obj->impulse_response = - (int32_t *)(src_obj->ring_buffers32 + - src_obj->num_channels * - (1 + src_obj->buffer_length)); - } else if (src_obj->bit_depth == 16) { - src_obj->impulse_response = - (int32_t *)(src_obj->ring_buffers16 + - src_obj->num_channels * - (1 + src_obj->buffer_length / 2)); - } - /* return ok, if everything worked out */ src_obj->is_initialised = true; return ASRC_EC_OK; @@ -486,7 +474,7 @@ enum asrc_error_code asrc_set_fs_ratio(struct comp_dev *dev, } /* Set the channel pointers and zero the buffers */ - error_code = initialise_buffer(src_obj); + error_code = initialise_buffer(dev, src_obj); /* check for errors */ if (error_code != ASRC_EC_OK) { comp_err(dev, "asrc_set_fs_ratio(), failed buffer initialise"); @@ -551,52 +539,51 @@ enum asrc_error_code asrc_set_output_format(struct comp_dev *dev, /* * BUFFER FUNCTIONS */ -static enum asrc_error_code initialise_buffer(struct asrc_farrow *src_obj) +static enum asrc_error_code initialise_buffer(struct comp_dev *dev, + struct asrc_farrow *src_obj) { - uint8_t *buffer; + int32_t *start_32; + int16_t *start_16; int ch; - int n; - - /* - * base_address points to the first address subsequently to the - * memory where the pointers to each ring buffer are stored. - */ - if (src_obj->bit_depth == 32) - buffer = (uint8_t *)(src_obj->ring_buffers32 + - src_obj->num_channels); - else if (src_obj->bit_depth == 16) - buffer = (uint8_t *)(src_obj->ring_buffers16 + - src_obj->num_channels); /* - * set buffer_length to filter_length * 2 to compensate for + * Set buffer_length to filter_length * 2 to compensate for * missing element wise wrap around while loading but allowing * aligned loads. */ src_obj->buffer_length = src_obj->filter_length * 2; + if (src_obj->buffer_length > ASRC_MAX_BUFFER_LENGTH) { + comp_err(dev, "initialise_buffer(), buffer_length %d exceeds max.", + src_obj->buffer_length); + return ASRC_EC_INVALID_BUFFER_LENGTH; + } + src_obj->buffer_write_position = src_obj->filter_length; - /* set the base addresses for every channel and initialise the - * buffers to zero + /* + * Initialize the dynamically allocated 2D array and clear the + * buffers to zero. */ if (src_obj->bit_depth == 32) { - for (ch = 0; ch < src_obj->num_channels; ch++) { - src_obj->ring_buffers32[ch] = ((int32_t *)buffer) + + start_32 = (int32_t *)(src_obj->ring_buffers32 + + src_obj->num_channels); + for (ch = 0; ch < src_obj->num_channels; ch++) + src_obj->ring_buffers32[ch] = start_32 + ch * src_obj->buffer_length; - /* initialise to zero */ - for (n = 0; n < src_obj->buffer_length; n++) - src_obj->ring_buffers32[ch][n] = 0; - } - } else if (src_obj->bit_depth == 16) { - for (ch = 0; ch < src_obj->num_channels; ch++) { - src_obj->ring_buffers16[ch] = ((int16_t *)buffer) + + /* initialise to zero */ + memset(start_32, 0, src_obj->num_channels * + src_obj->buffer_length * sizeof(int32_t)); + } else { + start_16 = (int16_t *)(src_obj->ring_buffers16 + + src_obj->num_channels); + for (ch = 0; ch < src_obj->num_channels; ch++) + src_obj->ring_buffers16[ch] = start_16 + ch * src_obj->buffer_length; - /* initialise to zero */ - for (n = 0; n < src_obj->buffer_length; n++) - src_obj->ring_buffers16[ch][n] = 0; - } + /* initialise to zero */ + memset(start_16, 0, src_obj->num_channels * + src_obj->buffer_length * sizeof(int16_t)); } return ASRC_EC_OK; @@ -764,6 +751,13 @@ static enum asrc_error_code initialise_filter(struct comp_dev *dev, return ASRC_EC_INVALID_SAMPLE_RATE; } + /* Check that filter length does not exceed allocated */ + if (src_obj->filter_length > ASRC_MAX_FILTER_LENGTH) { + comp_err(dev, "initialise_filter(), filter_length %d exceeds max", + src_obj->filter_length); + return ASRC_EC_INVALID_FILTER_LENGTH; + } + /* The function pointer is set according to the number of polyphase * filters */ diff --git a/src/include/sof/audio/asrc/asrc_farrow.h b/src/include/sof/audio/asrc/asrc_farrow.h index 2e9d0f2de981..27d5eb0020d5 100644 --- a/src/include/sof/audio/asrc/asrc_farrow.h +++ b/src/include/sof/audio/asrc/asrc_farrow.h @@ -46,6 +46,13 @@ #include #include +/* + * @brief FIR filter is max 128 taps and delay lines per channel are + * max 256 samples long. + */ +#define ASRC_MAX_FILTER_LENGTH 128 +#define ASRC_MAX_BUFFER_LENGTH 256 + /* * @brief Define whether the input and output buffers shall be * interleaved or not. @@ -136,8 +143,9 @@ enum asrc_error_code { /*!< control mode. */ ASRC_EC_FAILED_PUSH_MODE = -13, /*!< Push mode operation */ /*!< failed. */ - ASRC_EC_FAILED_PULL_MODE = -14 /*!< Pull mode operation */ + ASRC_EC_FAILED_PULL_MODE = -14, /*!< Pull mode operation */ /*!< failed. */ + ASRC_EC_INVALID_FILTER_LENGTH = -15, /*!< Length exceeds max. */ }; /* From c256109d72466a55620f3add51d729ff3176a2f7 Mon Sep 17 00:00:00 2001 From: Seppo Ingalsuo Date: Fri, 7 Aug 2020 14:25:28 +0300 Subject: [PATCH 2/2] Audio: ASRC: Reformat header file comments with longer lines The 80 characters split lines were hard to read. This change improves readability error codes enums. Signed-off-by: Seppo Ingalsuo --- src/include/sof/audio/asrc/asrc_farrow.h | 52 ++++++++---------------- 1 file changed, 18 insertions(+), 34 deletions(-) diff --git a/src/include/sof/audio/asrc/asrc_farrow.h b/src/include/sof/audio/asrc/asrc_farrow.h index 27d5eb0020d5..1af4adb7edd8 100644 --- a/src/include/sof/audio/asrc/asrc_farrow.h +++ b/src/include/sof/audio/asrc/asrc_farrow.h @@ -110,41 +110,25 @@ enum asrc_operation_mode { */ enum asrc_error_code { ASRC_EC_OK = 0, /*!< Operation successful. */ - ASRC_EC_INIT_FAILED = -1, /*!< Initialization of the */ - /*!< component failed. */ - ASRC_EC_UPDATE_FS_FAILED = -2, /*!< Control mode is set to */ - /*!< CM_FIXED and update */ - /*!< drift is not called in */ - /*!< time. */ - ASRC_EC_INVALID_POINTER = -3, /*!< Couldn't allocate */ - /*!< memory. Bad pointer. */ - ASRC_EC_INVALID_BUFFER_POINTER = -4, /*!< Internal buffer pointers */ - /*!< are invalid. */ - ASRC_EC_INVALID_SAMPLE_RATE = -5, /*!< Sample rate is not */ - /*!< supported. */ - ASRC_EC_INVALID_CONVERSION_RATIO = -6, /*!< Conversion ratio is not */ - /*!< supported. */ - ASRC_EC_INVALID_BIT_DEPTH = -7, /*!< Bit depth is not */ - /*!< supported. Choose either */ + ASRC_EC_INIT_FAILED = -1, /*!< Initialization of the component failed. */ + ASRC_EC_UPDATE_FS_FAILED = -2, /*!< Control mode is set to CM_FIXED and update */ + /*!< drift is not called in time. */ + ASRC_EC_INVALID_POINTER = -3, /*!< Couldn't allocate memory. Bad pointer. */ + ASRC_EC_INVALID_BUFFER_POINTER = -4, /*!< Internal buffer pointers are invalid. */ + ASRC_EC_INVALID_SAMPLE_RATE = -5, /*!< Sample rate is not supported. */ + ASRC_EC_INVALID_CONVERSION_RATIO = -6, /*!< Conversion ratio is not supported. */ + ASRC_EC_INVALID_BIT_DEPTH = -7, /*!< Bit depth is not supported. Choose either */ /*!< 16 or 32 bit. */ - ASRC_EC_INVALID_NUM_CHANNELS = -8, /*!< Nummber of channels must */ - /*!< be larger than zero. */ - ASRC_EC_INVALID_BUFFER_LENGTH = -9, /*!< Buffer length must be */ - /*!< larger than one. */ - ASRC_EC_INVALID_FRAME_SIZE = -10, /*!< Invalid frame size: must */ - /*!< be greater than 0 for */ - /*!< primary side and */ - /*!< secondary side. */ - ASRC_EC_INVALID_CLOCK_SKEW = -11, /*!< The clock drift is out */ - /*!< of bounds. */ - ASRC_EC_INVALID_CONTROL_MODE = -12, /*!< Call update_fs_ratio() */ - /*!< for feedback, and */ - /*!< update_drift() for fixed */ - /*!< control mode. */ - ASRC_EC_FAILED_PUSH_MODE = -13, /*!< Push mode operation */ - /*!< failed. */ - ASRC_EC_FAILED_PULL_MODE = -14, /*!< Pull mode operation */ - /*!< failed. */ + ASRC_EC_INVALID_NUM_CHANNELS = -8, /*!< Nummber of channels must be larger */ + /*!< than zero. */ + ASRC_EC_INVALID_BUFFER_LENGTH = -9, /*!< Buffer length must be larger than one. */ + ASRC_EC_INVALID_FRAME_SIZE = -10, /*!< Invalid frame size: must be greater than 0 */ + /*!< for primary side and secondary side. */ + ASRC_EC_INVALID_CLOCK_SKEW = -11, /*!< The clock drift is out of bounds. */ + ASRC_EC_INVALID_CONTROL_MODE = -12, /*!< Call update_fs_ratio() for feedback, and */ + /*!< update_drift() for fixed control mode. */ + ASRC_EC_FAILED_PUSH_MODE = -13, /*!< Push mode operation failed. */ + ASRC_EC_FAILED_PULL_MODE = -14, /*!< Pull mode operation failed. */ ASRC_EC_INVALID_FILTER_LENGTH = -15, /*!< Length exceeds max. */ };