ASoC: SOF/Intel/AMD/Mediatek/IMX: enable GCC '-fanalyzer' checks #4340

plbossart · 2023-05-05T21:39:53Z

Cleanups to work-around issues, prior to enable this static analysis by default.

It's pretty interesting that none of the other tools reported these issues....

@dbaluta @yaochunhung @kuanhsuncheng @crojewsk-intel @ujfalusi @ranj063 @marc-hb feedback welcome.

marc-hb · 2023-05-06T00:02:47Z

https://github.com/thesofproject/linux/actions/runs/4897771652/jobs/8746195783?pr=4340 failed like this:

gcc: error: unrecognized command line option ‘-fanalyzer’

Try an Ubuntu upgrade like this one:

[SKIP SOF-TEST] .github: upgrade most runners to latest Ubuntu 22.04 sof#7563

Even if not enough it won't hurt anyway!

marc-hb · 2023-05-06T00:09:16Z

It's not a problem today, since we do
check in the first use of the function, but static analysis tools are
not aware of the different ALSA stages. Rather than argue forever,
let's just add the error checks consistently and make sure this tool
can be added to the CI checks.

I like this pragmatic approach. I would add that it's NOT just to please static analyzers and silence false positives:

This also saves time for human code reviewers / "human analyzers"
Defensive programming makes the code more future proof. The conditions that are true today may be different tomorrow.

This is a false-positive, the GCC analyzer generated that report by
considering if (bytes->block) as true in some cases and false in
others.

This one looks more like a relatively simple bug that should be reported to GCC? Does it still show with cutting edge GCC? What is the most recent GCC version you tried?

plbossart · 2023-05-08T12:32:44Z

This '-fanalyzer' option was introduced with GCC 10

https://gcc.gnu.org/onlinedocs/gcc-10.1.0/gcc/Static-Analyzer-Options.html

I have GCC11 with Ubuntu 22.10

sound/soc/intel/atom/sst/sst_stream.c

marc-hb · 2023-05-08T18:06:42Z

Debian/Ubuntu support multiple gcc versions in a single distribution release BUT only one default version. For Ubuntu 20.04, the default gcc version was gcc 9.3: https://packages.ubuntu.com/focal/devel/

I see you've updated to ubuntu-latest which currently points at ubuntu 22.04 which fixes the failure. I would not point at ubuntu-latest because it may change at a very inconvenient moment, I would go for ubuntu-22.04 instead. Github has historically provided only LTS runners (used by 95% of Ubuntu users according to Ubuntu stats) so manually upgrading every 2 years seems like a much smaller and less risky inconvenience. No big deal, up to you!

https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners
https://ubuntu.com/about/release-cycle

plbossart · 2023-05-08T18:52:10Z

I would not point at ubuntu-latest because it may change at a very inconvenient moment, I would go for ubuntu-22.04 instead.

Yeah, that's what I had initially and then I changed my position. How would we know that something is broken with ubuntu-latest if we don't test it? In other words, let's use GitHub Actions to test GitHub actions. If things break we can always go back to the last version known to work.

marc-hb · 2023-05-08T19:29:33Z

How would we know that something is broken with ubuntu-latest if we don't test it?

I've never seen this being a real issue in any project. Linux distributions are very good with backwards compatibility, especially LTS releases. Now Ubuntu bugs and gcc bugs do exist of course (examples https://github.com/thesofproject/sof-test/discussions, thesofproject/sof#7114,...) but I've never seen any affect something as common as compiling the kernel.

The converse is much more common: starting to use some brand new features without realizing it. That's why it's good for CI to always lag some (reasonable) distance behind developers. CI effectively defines the minimum requirements.

Moreever CI never stays stuck on old releases for a very long time because some new fancy feature like -fanalyzer is eventually found missing which forces an upgrade: exactly what is happening here right now.

Just sharing my experience, again no big deal.

RanderWang · 2023-05-09T13:21:11Z

include/linux/firmware/imx/dsp.h

 	ipc->private_data = data;
 }

 static inline void *imx_dsp_get_data(struct imx_dsp_ipc *ipc)
 {
-	if (!ipc)
-		return NULL;


this change is not aligned with most of changes in this PR. Can we check ipc before it is used ?

I find this quite different, the other cases are more complicated and will have multiple cases and tests before returning NULL. Here it's just a straightforward test of something being NULL.

But that would be for @dbaluta @paulstelian97 to comment, I don't have any specific opinion on this code.

plbossart · 2023-05-16T16:14:35Z

No reply from @crojewsk-intel, dropped AVS patches and dropped AVS from the checks.

crojewsk-intel · 2023-05-17T14:03:56Z

Sorry for the late reply. My two cents (example based on bdw_rt286):

struct snd_soc_dai *codec_dai = snd_soc_card_get_codec_dai(card, "rt286-aif1");

if (!codec_dai)
	return 0;

in my opinion is an invalid scenario. Given the DAI LINKs declaration that is part of the machine board driver, such situation may never occur - if e.g.: rt286-aif1 is missing, sound card won't even be enumerated.

Another thing to consider is correctness against PM expectations. That is, on suspend we should not prevent the system-wide suspend so 0 shall be returned regardless but on resume that's not the case and errors should be propagated. Of course, there's another level of complication as results snd_soc_card_suspend_pre() and snd_soc_card_resume_post() are completely ignored by the framework. Oh well..

sound/soc/sof/intel/hda-dai.c

sound/soc/sof/topology.c

sound/soc/intel/atom/sst/sst_stream.c

.github/workflows/buildtest.yml

sound/soc/intel/atom/sst/sst_stream.c

plbossart · 2023-05-22T21:51:29Z

struct snd_soc_dai *codec_dai = snd_soc_card_get_codec_dai(card, "rt286-aif1");

if (!codec_dai)
	return 0;
in my opinion is an invalid scenario. Given the DAI LINKs declaration that is part of the machine board driver, such situation may never occur - if e.g.: rt286-aif1 is missing, sound card won't even be enumerated.

Static analysis can't know that. It just looks at the result of snd_soc_card_get_codec_dai() and tells you there is one possible case of NULL dereference.

…sis warnings make KCFLAGS='-fanalyzer' sound/soc/sof/ reports several NULL pointer dereference paths. sof_ipc4_probe_get_module_info() can return a NULL value, but it's only tested in the init state. Static analyzers cannot know the probe state machine and hence flags a potential issue for all calls of that function. Squelch these errors by adding the same check consistently. Signed-off-by: Pierre-Louis Bossart <[email protected]>

make KCFLAGS='-fanalyzer' sound/soc/sof/ reports an issue with memcpy: sound/soc/sof/ipc3.c: In function ‘ipc3_wait_tx_done’: sound/soc/sof/ipc3.c:309:33: error: use of NULL ‘reply_data’ where non-null expected [CWE-476] [-Werror=analyzer-null-argument] 309 | memcpy(reply_data, msg->reply_data, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 310 | msg->reply_size); The finding is legit with this call: return sof_ipc3_tx_msg(sdev, &pm_ctx, sizeof(pm_ctx), NULL, 0, false); Static analysis has no way of knowing that the reply will be zero-sized. Add a check to only do the memcpy if the reply size is not zero and the destination pointer is not NULL. Signed-off-by: Pierre-Louis Bossart <[email protected]>

make KCFLAGS='-fanalyzer' sound/soc/sof/intel/ reports a possible NULL pointer dereference. sound/soc/sof/topology.c:1136:21: error: dereference of NULL ‘w’ [CWE-476] [-Werror=analyzer-null-dereference] 1136 | strcmp(w->sname, rtd->dai_link->stream_name)) The code is rather confusing and can be simplified to make static analysis happy. No functionality change. Signed-off-by: Pierre-Louis Bossart <[email protected]>

imx_dsp_get_data() can return NULL, but the value is not checked before being usd, leading to static analysis warnings. sound/soc/sof/imx/imx8.c:85:32: error: dereference of NULL ‘0’ [CWE-476] [-Werror=analyzer-null-dereference] 85 | spin_lock_irqsave(&priv->sdev->ipc_lock, flags); | ~~~~^~~~~~ It appears this is not really a possible problem, so remove those checks. Signed-off-by: Pierre-Louis Bossart <[email protected]>

mtk_adsp_ipc_get_data() can return NULL, but the value is not checked before being used, leading to static analysis warnings. sound/soc/sof/mediatek/mt8195/mt8195.c:90:32: error: dereference of NULL ‘0’ [CWE-476] [-Werror=analyzer-null-dereference] 90 | spin_lock_irqsave(&priv->sdev->ipc_lock, flags); | ~~~~^~~~~~ It appears this is not really a possible problem, so remove those checks. Signed-off-by: Pierre-Louis Bossart <[email protected]>

snd_soc_card_get_codec_dai() can return NULL, but that value is not checked for, leading to false-positive static analysis warnings sound/soc/intel/boards/bdw_rt286.c:190:16: error: dereference of NULL ‘codec_dai’ [CWE-476] [-Werror=analyzer-null-dereference] 190 | return snd_soc_component_set_jack(codec_dai->component, NULL, NULL); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ‘card_suspend_pre’: event 1 | | 190 | return snd_soc_component_set_jack(codec_dai->component, NULL, NULL); | | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | | | | | (1) dereference of NULL ‘codec_dai’ This NULL dereference cannot happen, the codec-dai "rt286-aif1" must exists otherwise the card would not be created. Static analysis cannot know that however so we might as well squelch this report. Signed-off-by: Pierre-Louis Bossart <[email protected]>

make KCFLAGS='-fanalyzer' sound/soc/intel/atom/ reports a possible NULL pointer dereference. sound/soc/intel/atom/sst/sst_stream.c:221:40: error: dereference of NULL ‘block’ [CWE-476] [-Werror=analyzer-null-dereference] 221 | unsigned char *r = block->data; This is a false-positive, the GCC analyzer generated that report by considering if (bytes->block) as true in some cases and false in others. We can simplify the code and use a local variable so that static analysis does not try to look for cases where bytes->block can be modified concurrently. Signed-off-by: Pierre-Louis Bossart <[email protected]>

snd_soc_card_get_codec_dai() can return NULL, but that value is not checked for, leading to static analysis warnings. Signed-off-by: Pierre-Louis Bossart <[email protected]>

Not sure why were stayed with 20.04 for so long... Signed-off-by: Pierre-Louis Bossart <[email protected]>

make KCFLAGS='-fanalyzer' comes for free so we might as well use it, eh? Signed-off-by: Pierre-Louis Bossart <[email protected]>

plbossart · 2023-06-29T09:37:30Z

@dbaluta @paulstelian97 can I ask you to look at the IMX part?

@yaochunhung @kuanhsuncheng can I ask you to look at the Mediatek part (same concept as IMX)?

plbossart · 2023-07-11T10:36:18Z

Unless I see a sustained objection, I am going to merge this series by the end of the week.

I don't see any reason why we should not use a tool because of lack of comments/approvals. This PR has been around for 2 months, time to move forward.

@ranj063 @ujfalusi @bardliao @dbaluta @paulstelian97 @yaochunhung @kuanhsuncheng @bhiregoudar FYI.

vijendarmukunda

LGTM

dbaluta · 2023-07-11T10:56:06Z

Looks good to me.

yaochunhung

LGTM. Thanks.

marc-hb · 2023-07-11T13:29:23Z

In thesofproject/sof#7562 we found that there is apparently no way to manage false positives.

plbossart force-pushed the fix/gcc-fanalyzer branch from 3c9b6ae to 7ef818d Compare May 8, 2023 12:38

paulstelian97 reviewed May 8, 2023

View reviewed changes

sound/soc/intel/atom/sst/sst_stream.c Outdated Show resolved Hide resolved

plbossart force-pushed the fix/gcc-fanalyzer branch 2 times, most recently from 98e6f21 to 5a3c647 Compare May 8, 2023 15:53

plbossart marked this pull request as ready for review May 8, 2023 15:54

plbossart requested review from libinyang, bardliao, RanderWang, ranj063, singalsu, juimonen, lgirdwood, kv2019i, dbaluta, ujfalusi, ajitkupandey, kuanhsuncheng and yaochunhung as code owners May 8, 2023 15:54

marc-hb mentioned this pull request May 8, 2023

module_adapter.c: warning: dereference of NULL: mod->output_buffers[i].data thesofproject/sof#7562

Open

marc-hb requested a review from andyross May 8, 2023 21:07

RanderWang reviewed May 9, 2023

View reviewed changes

plbossart force-pushed the fix/gcc-fanalyzer branch from 5a3c647 to bb5385e Compare May 16, 2023 16:13

plbossart requested a review from RanderWang May 16, 2023 16:14

RanderWang previously approved these changes May 18, 2023

View reviewed changes

ranj063 reviewed May 18, 2023

View reviewed changes

sound/soc/sof/intel/hda-dai.c Outdated Show resolved Hide resolved

sound/soc/sof/topology.c Show resolved Hide resolved

sound/soc/intel/atom/sst/sst_stream.c Show resolved Hide resolved

marc-hb reviewed May 18, 2023

View reviewed changes

.github/workflows/buildtest.yml Outdated Show resolved Hide resolved

sound/soc/intel/atom/sst/sst_stream.c Show resolved Hide resolved

plbossart dismissed RanderWang’s stale review via e97bbb8 May 18, 2023 13:52

RanderWang approved these changes May 19, 2023

View reviewed changes

plbossart requested a review from ranj063 May 22, 2023 21:53

plbossart force-pushed the fix/gcc-fanalyzer branch from e97bbb8 to 45bb03b Compare May 22, 2023 22:01

plbossart mentioned this pull request May 30, 2023

Fix/hda dai ops #4394

Merged

plbossart added 10 commits June 29, 2023 11:33

ASoC: amd: acp5x-mach:add checks to avoid static analysis warnings

40c1f50

snd_soc_card_get_codec_dai() can return NULL, but that value is not checked for, leading to static analysis warnings. Signed-off-by: Pierre-Louis Bossart <[email protected]>

[GITHUB ACTIONS] bump version to ubuntu-latest

ffd00f1

Not sure why were stayed with 20.04 for so long... Signed-off-by: Pierre-Louis Bossart <[email protected]>

[GITHUB ACTIONS] add GCC static analyzer

1347f6f

make KCFLAGS='-fanalyzer' comes for free so we might as well use it, eh? Signed-off-by: Pierre-Louis Bossart <[email protected]>

plbossart force-pushed the fix/gcc-fanalyzer branch from 45bb03b to 1347f6f Compare June 29, 2023 09:35

vijendarmukunda approved these changes Jul 11, 2023

View reviewed changes

dbaluta approved these changes Jul 11, 2023

View reviewed changes

yaochunhung approved these changes Jul 11, 2023

View reviewed changes

paulstelian97 removed their request for review July 11, 2023 16:02

plbossart merged commit 64c70e6 into thesofproject:topic/sof-dev Jul 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ASoC: SOF/Intel/AMD/Mediatek/IMX: enable GCC '-fanalyzer' checks #4340

ASoC: SOF/Intel/AMD/Mediatek/IMX: enable GCC '-fanalyzer' checks #4340

plbossart commented May 5, 2023

marc-hb commented May 6, 2023

marc-hb commented May 6, 2023

plbossart commented May 8, 2023 •

edited

Loading

marc-hb commented May 8, 2023 •

edited

Loading

plbossart commented May 8, 2023

marc-hb commented May 8, 2023

RanderWang May 9, 2023

plbossart May 9, 2023

plbossart commented May 16, 2023

crojewsk-intel commented May 17, 2023

plbossart commented May 22, 2023

plbossart commented Jun 29, 2023

plbossart commented Jul 11, 2023 •

edited

Loading

vijendarmukunda left a comment

dbaluta commented Jul 11, 2023

yaochunhung left a comment

marc-hb commented Jul 11, 2023

ASoC: SOF/Intel/AMD/Mediatek/IMX: enable GCC '-fanalyzer' checks #4340

ASoC: SOF/Intel/AMD/Mediatek/IMX: enable GCC '-fanalyzer' checks #4340

Conversation

plbossart commented May 5, 2023

marc-hb commented May 6, 2023

marc-hb commented May 6, 2023

plbossart commented May 8, 2023 • edited Loading

marc-hb commented May 8, 2023 • edited Loading

plbossart commented May 8, 2023

marc-hb commented May 8, 2023

RanderWang May 9, 2023

Choose a reason for hiding this comment

plbossart May 9, 2023

Choose a reason for hiding this comment

plbossart commented May 16, 2023

crojewsk-intel commented May 17, 2023

plbossart commented May 22, 2023

plbossart commented Jun 29, 2023

plbossart commented Jul 11, 2023 • edited Loading

vijendarmukunda left a comment

Choose a reason for hiding this comment

dbaluta commented Jul 11, 2023

yaochunhung left a comment

Choose a reason for hiding this comment

marc-hb commented Jul 11, 2023

plbossart commented May 8, 2023 •

edited

Loading

marc-hb commented May 8, 2023 •

edited

Loading

plbossart commented Jul 11, 2023 •

edited

Loading