Skip to content

Static code analysis#6858

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lgirdwood merged 5 commits into from
Dec 21, 2022
Merged

Static code analysis #6858

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ee2208
copier: avoid array overflow
lyakh Dec 20, 2022
2e6053a
volume: fix a potentially uninitialised array
lyakh Dec 20, 2022
dfcc4fb
selector: avoid array overflow
lyakh Dec 20, 2022
8adb9ac
logger: check localtime() return value
lyakh Dec 20, 2022
ce00098
ipc: ipc4: do not access memory beyond object boudaries
lyakh Dec 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/audio/copier/copier.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -796,6 +796,8 @@ static int do_conversion_copy(struct comp_dev *dev,
comp_get_copy_limits(src, sink, processed_data);

i = IPC4_SINK_QUEUE_ID(sink->id);
if (i >= IPC4_COPIER_MODULE_OUTPUT_PINS_COUNT)
return -EINVAL;
buffer_stream_invalidate(src, processed_data->source_bytes);

cd->converter[i](&src->stream, 0, &sink->stream, 0,
Expand Down
2 changes: 1 addition & 1 deletion src/audio/module_adapter/module/volume/volume.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -552,7 +552,7 @@ static int volume_init(struct processing_module *mod)
uint32_t instance_id;

channels_count = mod->priv.cfg.base_cfg.audio_fmt.channels_count;
if (channels_count > SOF_IPC_MAX_CHANNELS) {
if (channels_count > SOF_IPC_MAX_CHANNELS || !channels_count) {
comp_err(dev, "volume_init(): Invalid channels count %u", channels_count);
return -EINVAL;
}
Expand Down
8 changes: 6 additions & 2 deletions src/audio/selector/selector_generic.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -216,6 +216,8 @@ static void sel_s16le(struct processing_module *mod, struct input_stream_buffer
int processed = 0;
int source_frame_bytes = audio_stream_frame_bytes(source);
int sink_frame_bytes = audio_stream_frame_bytes(sink);
int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, source->channels);
int n_chan_sink = MIN(SEL_SINK_CHANNELS_MAX, sink->channels);

while (processed < frames) {
n = frames - processed;
Expand All@@ -224,7 +226,7 @@ static void sel_s16le(struct processing_module *mod, struct input_stream_buffer
nmax = audio_stream_bytes_without_wrap(sink, dest) / sink_frame_bytes;
n = MIN(n, nmax);
for (i = 0; i < n; i++) {
process_frame_s16le(dest, sink->channels, src, source->channels,
process_frame_s16le(dest, n_chan_sink, src, n_chan_source,
&cd->coeffs_config);
src += source->channels;
dest += sink->channels;
Expand DownExpand Up@@ -285,6 +287,8 @@ static void sel_s32le(struct processing_module *mod, struct input_stream_buffer
int processed = 0;
int source_frame_bytes = audio_stream_frame_bytes(source);
int sink_frame_bytes = audio_stream_frame_bytes(sink);
int n_chan_source = MIN(SEL_SOURCE_CHANNELS_MAX, source->channels);
int n_chan_sink = MIN(SEL_SINK_CHANNELS_MAX, sink->channels);

while (processed < frames) {
n = frames - processed;
Expand All@@ -293,7 +297,7 @@ static void sel_s32le(struct processing_module *mod, struct input_stream_buffer
nmax = audio_stream_bytes_without_wrap(sink, dest) / sink_frame_bytes;
n = MIN(n, nmax);
for (i = 0; i < n; i++) {
process_frame_s32le(dest, sink->channels, src, source->channels,
process_frame_s32le(dest, n_chan_sink, src, n_chan_source,
&cd->coeffs_config);
src += source->channels;
dest += sink->channels;
Expand Down
24 changes: 11 additions & 13 deletions src/ipc/ipc4/handler.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -465,7 +465,7 @@ static int ipc4_process_chain_dma(struct ipc4_message_request *ipc4)
bool delay = false;
int ret;

memcpy_s(&cdma, sizeof(cdma), ipc4, sizeof(cdma));
memcpy_s(&cdma, sizeof(cdma), ipc4, sizeof(*ipc4));

@paulstelian97paulstelian97Dec 21, 2022•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an unchecked memcpy_s call which may fail and I believe in case of failure there's an issue (the destination gets zeroed out, so you need to pay attention to that to avoid bugs like this). If this returns an error you should propagate or assert it doesn't happen (using the non-removable SOF asserts). Ditto on all other memcpy_s calls in this patch.

The reason I'm requesting changes is that if this happens the bug is silent and not obvious. I hate that kind of bug.

@lyakhlyakhDec 21, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this hasn't been changed by this commit. This commit doesn't increase chances of memcpy_s() to fail

@lgirdwoodlgirdwoodDec 21, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we take the memcpy_s check as a separate patch/PR. This needs to be fixed first.


if (cdma.primary.r.allocate && cdma.extension.r.fifo_size) {
ret = ipc4_create_chain_dma(ipc, &cdma);
Expand DownExpand Up@@ -581,11 +581,12 @@ static int ipc4_process_glb_message(struct ipc4_message_request *ipc4)

static int ipc4_init_module_instance(struct ipc4_message_request *ipc4)
{
struct ipc4_module_init_instance module;
struct ipc4_module_init_instance module = {};
struct sof_ipc_comp comp;
struct comp_dev *dev;

memcpy_s(&module, sizeof(module), ipc4, sizeof(module));
/* we only need the common header here, all we have from the IPC */
memcpy_s(&module, sizeof(module), ipc4, sizeof(*ipc4));
tr_dbg(&ipc_tr, "ipc4_init_module_instance %x : %x", (uint32_t)module.primary.r.module_id,
(uint32_t)module.primary.r.instance_id);

Expand DownExpand Up@@ -614,7 +615,7 @@ static int ipc4_bind_module_instance(struct ipc4_message_request *ipc4)
struct ipc4_module_bind_unbind bu;
struct ipc *ipc = ipc_get();

memcpy_s(&bu, sizeof(bu), ipc4, sizeof(bu));
memcpy_s(&bu, sizeof(bu), ipc4, sizeof(*ipc4));
tr_dbg(&ipc_tr, "ipc4_bind_module_instance %x : %x with %x : %x",
(uint32_t)bu.primary.r.module_id, (uint32_t)bu.primary.r.instance_id,
(uint32_t)bu.extension.r.dst_module_id, (uint32_t)bu.extension.r.dst_instance_id);
Expand All@@ -627,7 +628,7 @@ static int ipc4_unbind_module_instance(struct ipc4_message_request *ipc4)
struct ipc4_module_bind_unbind bu;
struct ipc *ipc = ipc_get();

memcpy_s(&bu, sizeof(bu), ipc4, sizeof(bu));
memcpy_s(&bu, sizeof(bu), ipc4, sizeof(*ipc4));
tr_dbg(&ipc_tr, "ipc4_unbind_module_instance %x : %x with %x : %x",
(uint32_t)bu.primary.r.module_id, (uint32_t)bu.primary.r.instance_id,
(uint32_t)bu.extension.r.dst_module_id, (uint32_t)bu.extension.r.dst_instance_id);
Expand All@@ -646,7 +647,7 @@ static int ipc4_get_large_config_module_instance(struct ipc4_message_request *ip
void *response_buffer;
int ret;

memcpy_s(&config, sizeof(config), ipc4, sizeof(config));
memcpy_s(&config, sizeof(config), ipc4, sizeof(*ipc4));
tr_dbg(&ipc_tr, "ipc4_get_large_config_module_instance %x : %x",
(uint32_t)config.primary.r.module_id, (uint32_t)config.primary.r.instance_id);

Expand DownExpand Up@@ -719,7 +720,7 @@ static int ipc4_set_large_config_module_instance(struct ipc4_message_request *ip
const struct comp_driver *drv;
int ret;

memcpy_s(&config, sizeof(config), ipc4, sizeof(config));
memcpy_s(&config, sizeof(config), ipc4, sizeof(*ipc4));
dcache_invalidate_region((__sparse_force void __sparse_cache *)MAILBOX_HOSTBOX_BASE,
config.extension.r.data_off_size);
tr_dbg(&ipc_tr, "ipc4_set_large_config_module_instance %x : %x",
Expand DownExpand Up@@ -767,7 +768,7 @@ static int ipc4_delete_module_instance(struct ipc4_message_request *ipc4)
uint32_t comp_id;
int ret;

memcpy_s(&module, sizeof(module), ipc4, sizeof(module));
memcpy_s(&module, sizeof(module), ipc4, sizeof(*ipc4));
tr_dbg(&ipc_tr, "ipc4_delete_module_instance %x : %x", (uint32_t)module.primary.r.module_id,
(uint32_t)module.primary.r.instance_id);

Expand All@@ -789,7 +790,7 @@ static int ipc4_module_process_d0ix(struct ipc4_message_request *ipc4)
struct ipc4_module_set_d0ix d0ix;
uint32_t module_id, instance_id;

memcpy_s(&d0ix, sizeof(d0ix), ipc4, sizeof(d0ix));
memcpy_s(&d0ix, sizeof(d0ix), ipc4, sizeof(*ipc4));
module_id = d0ix.primary.r.module_id;
instance_id = d0ix.primary.r.instance_id;

Expand DownExpand Up@@ -818,7 +819,7 @@ static int ipc4_module_process_dx(struct ipc4_message_request *ipc4)
uint32_t core_id;
int ret;

memcpy_s(&dx, sizeof(dx), ipc4, sizeof(dx));
memcpy_s(&dx, sizeof(dx), ipc4, sizeof(*ipc4));
module_id = dx.primary.r.module_id;
instance_id = dx.primary.r.instance_id;

Expand DownExpand Up@@ -1040,9 +1041,6 @@ void ipc_cmd(struct ipc_cmd_hdr *_hdr)
enum ipc4_message_target target;
int err;

if (!in)
return;

if (cpu_is_primary(cpu_get_id()))
tr_info(&ipc_tr, "rx\t: %#x|%#x", in->primary.dat, in->extension.dat);

Expand Down
7 changes: 5 additions & 2 deletions tools/logger/convert.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -359,11 +359,14 @@ static inline void print_table_header(void)
fprintf(out_fd, "%s", "CONTENT");

if (global_config->time_precision >= 0) {
struct tm *ltime = localtime(&epoc_secs);

/* e.g.: ktime=4263.487s @ 2021-04-27 14:21:13 -0700 PDT */
fprintf(out_fd, "\tktime=%lu.%03lus",
ktime.tv_sec, ktime.tv_nsec / 1000000);
if (strftime(date_string, sizeof(date_string),
"%F %X %z %Z", localtime(&epoc_secs)))

if (ltime && strftime(date_string, sizeof(date_string),
"%F %X %z %Z", ltime))

@marc-hbmarc-hbDec 20, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would make the output look weird if ltime is ever NULL but ltime is never going to be NULL because epoc_secs comes straight from time() above which never fails - static analysis just does not know that.

Whatever is required to please the static analysis Gods.

fprintf(out_fd, " @ %s", date_string);
}

Expand Down