Dummy smart_amp: copy input_pins and output_pin separately #7588
Conversation
andrula-song
changed the title
| memcpy_s(sad->ipc4_cfg.input_pins, in_size, | ||
| base_cfg->base_cfg_ext.pin_formats, in_size); | ||
| memcpy_s(&sad->ipc4_cfg.output_pin, out_size, | ||
| base_cfg->base_cfg_ext.pin_formats + in_size, out_size); |
There was a problem hiding this comment.
Pointer arithmetic on a flexible array member, wow! I wonder what that means. It looks suspicious... maybe this instead?
| base_cfg->base_cfg_ext.pin_formats + in_size, out_size); | |
| &base_cfg->base_cfg_ext.pin_formats[0] + in_size, out_size); |
There was a problem hiding this comment.
Pointer arithmetic on a flexible array member, wow! I wonder what that means.
Most likely the same thing as a regular array, sorry the noise. In that case:
| base_cfg->base_cfg_ext.pin_formats + in_size, out_size); | |
| &base_cfg->base_cfg_ext.pin_formats[in_size], out_size); |
There was a problem hiding this comment.
this may cause memory leak, it may add pointer based on pin_format size instead of byte, better continue add (char *) at beginning in case xtensa compiler is not so strong.
(char *)&base_cfg->base_cfg_ext.pin_formats[0] + in_size
or
(char *)&base_cfg->base_cfg_ext.pin_formats[1] ?
There was a problem hiding this comment.
in case xtensa compiler is not so strong.
The xtensa compiler is based on either GCC or Clang so there's no standard compliance concern.
There was a problem hiding this comment.
this may cause memory leak,
I think you mean buffer overflow. Memory leak is something entirely different.
add (char *) at beginning
Yes the code assumes that sizeof(pin_formats[0]) is 1 and that's not very obvious. Especially not considering input_pins and output_pin are totally different.
There was a problem hiding this comment.
sorry, should be buffer overflow.
Thanks. |
The original code confuses static code analyzers since it was relying on the fact that we have the 2x input and 1x output pin config in adjacent position in smart_amp_data struct similarly to the extended module configuration. While it works, it is not a good practice. Split the copy of input and output pin formats to make the code obvious and less error prone. Fixes: andrula-song@ bcc1407 ("smart_amp_test: Split the module config and blob receiving for IPC4") Signed-off-by: Andrula Song <andrula.song@intel.com>
andrula-song
requested review from
lgirdwood,
plbossart,
mmaka1,
lbetlej and
dbaluta
as code owners
|
SPFCI TEST |
|
SOFCI TEST |
1 similar comment
|
SOFCI TEST |
|
One fail hit in https://sof-ci.01.org/sofpr/PR7588/build7818/devicetest/index.html -- a known bug. |
The original code confuses static code analyzers since it was
relying on the fact that we have the 2x input and 1x output pin
config in adjacent position in smart_amp_data struct similarly
to the extended module configuration.
While it works, it is not a good practice.
Split the copy of input and output pin formats to make the code
obvious and less error prone.
Fixes: andrula-song@bcc1407 ("smart_amp_test: Split the module config and blob receiving for IPC4")
Signed-off-by: Andrula Song andrula.song@intel.com