Static code analysis #6858

lyakh · 2022-12-20T13:44:29Z

more fixes

paulstelian97 · 2022-12-20T13:56:06Z

src/audio/selector/selector_generic.c

I actually wonder if this is what should be done. I'd consider returning an error or clamping the value to the maximum allowed one here.

@lyakh is this aligning with the other frame formats in selector or should this be done by the caller ?

this functions returns void

@lyakh I mean do it here in the caller. This way we can check all users.

static int selector_process(struct processing_module *mod, struct input_stream_buffer *input_buffers, int num_input_buffers, struct output_stream_buffer *output_buffers, int num_output_buffers) { struct comp_data *cd = module_get_private_data(mod); uint32_t avail_frames = input_buffers[0].size; comp_dbg(mod->dev, "selector_process()"); if (!avail_frames) return PPL_STATUS_PATH_STOP; /* copy selected channels from in to out */ cd->sel_func(mod, input_buffers, output_buffers, avail_frames); return 0; }

@lgirdwood I've done almost that. Firstly don't think static analysers would be smart enough to look that far, so we'd have to ignore their complains. Secondly only multi-channel selector processing needs this limitation, not single-channel versions

Its a cheap check here regardless of single/multi channel and static analysis can be closed for this one.

paulstelian97 · 2022-12-20T13:57:51Z

tools/logger/convert.c

Technically the second one doesn't point to the stack (so it's technically unneeded), but I'd say setting it to null is possibly fine here so I don't actually have an issue with this.

global_config is not owned by this function so this looks wrong and possibly buggy. Did you test this? sof-logger is not used by Intel on this branch.

technically inside this function we don't know where the config parameter comes from, but in fact it is on stack in the caller's function. A better fix would be to get rid of those global variables and to pass config as a parameter to all functions.

@marc-hb this function is called only in one place. And after it completes main() completes too.

paulstelian97 · 2022-12-20T13:59:00Z

tools/logger/logger.c

I've seen another change to this part of the file already (similar in spirit, even). Try not to duplicate work! (edit: not quite, the other PR touches stuff 3 lines above)

paulstelian97

Blocking due to conflicting change with #6846

Change isn't actually conflicting, the other PR touches something 3 lines above this one. Not yet approving outright due to the other comments.

lgirdwood

Looks good, but need to check selector.c for right place for src/dst count.

lgirdwood · 2022-12-20T14:13:59Z

src/audio/selector/selector_generic.c

@lyakh is this aligning with the other frame formats in selector or should this be done by the caller ?

kv2019i

Good fixes @lyakh ! One alternative implementation proposal in inline comments, but no showstoppers. I think these can go in as they are.

kv2019i · 2022-12-20T17:30:48Z

tools/logger/convert.c

@lyakh I guess one alternative would be to make "struct snd_sof_Logs_header snd" a static global variable. Not much cleaner, but potentially less lines to change.

I'd much rather avoid adding more global variables. We already have a couple in the logger and I think we should remove them

marc-hb

A better fix would be to get rid of those global variables and to pass config as a parameter to all functions.

That was more or less the case in the past. Then commit 5b29dae said "enough spaghetti code, let's just use a single global config". But it stopped in the middle of the bridge and left this mess.

I'm trying now a simpler fix that finishes the job and makes a single, global config with a minimal number of lines changed.

Please resubmit this PR without this commit, this PR has too many unrelated commits anyway.

marc-hb · 2022-12-20T18:47:14Z

tools/logger/convert.c

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.

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not finish, leaving behind a supposedly "global" variable that is actually a confusing global pointer to a struct local to the main() function. This confuses some static analyzer complaining that stack values are being returned, see thesofproject#6858 and thesofproject#6738. This is a false positive because the main()'s stack lifespan is the same as a global but let's simplify things anyway. Also stop using 'extern' in .c files, use a proper .h file instead. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

marc-hb · 2022-12-20T22:47:27Z

I'm trying now a simpler fix that finishes the job and makes a single, global config with a minimal number of lines changed.

Done:

logger: make "global_config" truly global #6862

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not complete, leaving a confusing mix of globals and locals. This confuses some static analyzer complaining that stack values are being returned, see thesofproject#6858 and thesofproject#6738. This is a false positive because convert's() stack lifespan is practically the same as a global but let's simplify things anyway. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

See the more focused, 6862 alternative

Fix a case of a possible array overflow, detected by static code analysis. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Make sure there's at least one channel in the volume configuration. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Check source and sink channels to avoid accessing beyond array boundaries. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

localtime() can return NULL in error cases. Check the return value before dereferencing it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

With IPC4 only two 32-bit values are copied from the host in the common part, anything beyond that must be copied separately. Those two values are stored in a global msg_data.msg_in object. Therefore always when reading from it only up to 8 bytes should be read. In most cases it is also the case, because the target object has the same size, but in case of SOF_IPC4_MOD_INIT_INSTANCE this isn't the case, there special care must be taken. Also remove a redundant NULL check where NULL cannot appear. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

paulstelian97 · 2022-12-21T08:50:25Z

src/ipc/ipc4/handler.c

 	int ret;

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


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.

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

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

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not finish, leaving behind a supposedly "global" variable that is actually a confusing global pointer to a struct local to the main() function. This confuses some static analyzer complaining that stack values are being returned, see #6858 and #6738. This is a false positive because the main()'s stack lifespan is the same as a global but let's simplify things anyway. Also stop using 'extern' in .c files, use a proper .h file instead. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not complete, leaving a confusing mix of globals and locals. This confuses some static analyzer complaining that stack values are being returned, see #6858 and #6738. This is a false positive because convert's() stack lifespan is practically the same as a global but let's simplify things anyway. Signed-off-by: Marc Herbert <marc.herbert@intel.com>

lgirdwood

@mwasko fyi - pls cherry-pick for mtl if needed.
@lyakh can you circle back and update the memcpy_s with checks in another PR.

New PR will be created to address updates requested for memcpy_s.

mwasko · 2022-12-21T12:59:57Z

@mwasko fyi - pls cherry-pick for mtl if needed.

@marcinszkudlinski, @abonislawski FYI

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not finish, leaving behind a supposedly "global" variable that is actually a confusing global pointer to a struct local to the main() function. This confuses some static analyzer complaining that stack values are being returned, see thesofproject#6858 and thesofproject#6738. This is a false positive because the main()'s stack lifespan is the same as a global but let's simplify things anyway. Also stop using 'extern' in .c files, use a proper .h file instead. Signed-off-by: Marc Herbert <marc.herbert@intel.com> (cherry picked from commit 327a26b) Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not complete, leaving a confusing mix of globals and locals. This confuses some static analyzer complaining that stack values are being returned, see thesofproject#6858 and thesofproject#6738. This is a false positive because convert's() stack lifespan is practically the same as a global but let's simplify things anyway. Signed-off-by: Marc Herbert <marc.herbert@intel.com> (cherry picked from commit 2dfaee6) Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not finish, leaving behind a supposedly "global" variable that is actually a confusing global pointer to a struct local to the main() function. This confuses some static analyzer complaining that stack values are being returned, see #6858 and #6738. This is a false positive because the main()'s stack lifespan is the same as a global but let's simplify things anyway. Also stop using 'extern' in .c files, use a proper .h file instead. Signed-off-by: Marc Herbert <marc.herbert@intel.com> (cherry picked from commit 327a26b) Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not complete, leaving a confusing mix of globals and locals. This confuses some static analyzer complaining that stack values are being returned, see #6858 and #6738. This is a false positive because convert's() stack lifespan is practically the same as a global but let's simplify things anyway. Signed-off-by: Marc Herbert <marc.herbert@intel.com> (cherry picked from commit 2dfaee6) Signed-off-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not finish, leaving behind a supposedly "global" variable that is actually a confusing global pointer to a struct local to the main() function. This confuses some static analyzer complaining that stack values are being returned, see thesofproject#6858 and thesofproject#6738. This is a false positive because the main()'s stack lifespan is the same as a global but let's simplify things anyway. Also stop using 'extern' in .c files, use a proper .h file instead. Signed-off-by: Marc Herbert <marc.herbert@intel.com> (cherry picked from commit 327a26b)

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not complete, leaving a confusing mix of globals and locals. This confuses some static analyzer complaining that stack values are being returned, see thesofproject#6858 and thesofproject#6738. This is a false positive because convert's() stack lifespan is practically the same as a global but let's simplify things anyway. Signed-off-by: Marc Herbert <marc.herbert@intel.com> (cherry picked from commit 2dfaee6)

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not finish, leaving behind a supposedly "global" variable that is actually a confusing global pointer to a struct local to the main() function. This confuses some static analyzer complaining that stack values are being returned, see #6858 and #6738. This is a false positive because the main()'s stack lifespan is the same as a global but let's simplify things anyway. Also stop using 'extern' in .c files, use a proper .h file instead. Signed-off-by: Marc Herbert <marc.herbert@intel.com> (cherry picked from commit 327a26b)

Finish the job that commit 5b29dae ("logger: Create global convert_config variable to avoid spaghetti code.") started but did not complete, leaving a confusing mix of globals and locals. This confuses some static analyzer complaining that stack values are being returned, see #6858 and #6738. This is a false positive because convert's() stack lifespan is practically the same as a global but let's simplify things anyway. Signed-off-by: Marc Herbert <marc.herbert@intel.com> (cherry picked from commit 2dfaee6)

lyakh requested review from akloniex, bkokoszx, dbaluta, lbetlej, lgirdwood, mmaka1 and plbossart as code owners December 20, 2022 13:44

paulstelian97 reviewed Dec 20, 2022

View reviewed changes

paulstelian97 previously requested changes Dec 20, 2022

View reviewed changes

lyakh added the P1 Blocker bugs or important features label Dec 20, 2022

lgirdwood reviewed Dec 20, 2022

View reviewed changes

lyakh force-pushed the static branch from d05eaa7 to 4fe7346 Compare December 20, 2022 16:32

kv2019i approved these changes Dec 20, 2022

View reviewed changes

marc-hb previously requested changes Dec 20, 2022

View reviewed changes

marc-hb requested a review from paulstelian97 December 20, 2022 20:02

marc-hb mentioned this pull request Dec 20, 2022

logger: make "global_config" truly global #6862

Merged

lyakh added 5 commits December 21, 2022 09:18

copier: avoid array overflow

0ee2208

Fix a case of a possible array overflow, detected by static code analysis. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

volume: fix a potentially uninitialised array

2e6053a

Make sure there's at least one channel in the volume configuration. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

selector: avoid array overflow

dfcc4fb

Check source and sink channels to avoid accessing beyond array boundaries. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

logger: check localtime() return value

8adb9ac

localtime() can return NULL in error cases. Check the return value before dereferencing it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the static branch from 4fe7346 to ce00098 Compare December 21, 2022 08:18

kv2019i requested a review from lgirdwood December 21, 2022 08:21

kv2019i requested a review from marc-hb December 21, 2022 08:21

paulstelian97 previously requested changes Dec 21, 2022

View reviewed changes

lgirdwood approved these changes Dec 21, 2022

View reviewed changes

lgirdwood merged commit 96686cf into thesofproject:main Dec 21, 2022

lyakh deleted the static branch December 21, 2022 10:13

kv2019i mentioned this pull request Dec 21, 2022

[SOF2.4] Backport fixes PR6848, PR6858 and PR6862 #6871

Merged

Static code analysis #6858

Static code analysis #6858

Uh oh!

Conversation

lyakh commented Dec 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulstelian97 Dec 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulstelian97 left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-hb commented Dec 20, 2022

Uh oh!

paulstelian97 Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

mwasko commented Dec 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

paulstelian97 Dec 20, 2022 •

edited

Loading

paulstelian97 Dec 21, 2022 •

edited

Loading