linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* uninitialized variables bugs
@ 2022-05-06  9:13 Dan Carpenter
  2022-05-06  9:53 ` [Ksummit-discuss] " Julia Lawall
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Dan Carpenter @ 2022-05-06  9:13 UTC (permalink / raw)
  To: ksummit-discuss, linux-kernel; +Cc: Nathan Chancellor, kbuild, lkp

Ever since commit 78a5255ffb6a ("Stop the ad-hoc games with
-Wno-maybe-initialized"), GCC's uninitialized variable warnings have
been disabled by default.  Now, you have to turn on W=1 or W=2 to see
the warnings which nobody except Arnd does.

Disabling that has lead to a bunch of embarrassing bugs where variables
are *never* initialized.  Very unsubtle bugs.  The bugs doesn't reach
users because Nathan Chancellor and I review Clang and Smatch warnings
respectively.  Also the kbuild-bot reports uninitialized variables.

It's a lot to deal with.  Uninitialized variable bugs are probably the
most common bug I have to deal with.

It's frustrating.  Sometimes the false positives are hard to analyse
because I have to read through multiple functions.  A lot of times
when I write a patch and a commit message Nathan has already fixed it
so it's just a waste of time.

It's risky as well.  The Smatch check for uninitialized variables was
broken for most of 2021.  Nathan sometimes goes on vacation.

I guess I would hope that one day we can turn on the GCC uninitialized
variable warnings again.  That would mean silencing false positives
which a lot of people don't want to do...  Maybe Clang has fewer false
positives than GCC?

The Smatch check for uninitialized variable was deliberately written to
be more strict than GCC because GCC was missing bugs.  So I think
leaving Smatch false positives is fine.  There is a trade off between
fewer false positives and missing bugs and Smatch is meant to err on the
side of finding bugs but with the cost of false positives.

Most of the Smatch uninitialized false positives are caused by loops:

	int i, ret;

	for (i = 0; i < bytes; i++) { // <-- what if bytes is zero?
		if (...)
			continue; // <-- can every iteration hit continue?
		ret = frob();
	}

	return ret;

There is also stuff like this which is harmless:

	uint val;

	ret = read(&val);
	*p = val;  // <-- uninitialized variable if read() fails
	return ret;

Btw, here is how to run Smatch on your code:
https://staticthinking.wordpress.com/2022/04/25/how-to-run-smatch-on-your-code/

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Ksummit-discuss] uninitialized variables bugs
  2022-05-06  9:13 uninitialized variables bugs Dan Carpenter
@ 2022-05-06  9:53 ` Julia Lawall
  2022-05-06 11:56 ` Arnd Bergmann
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Julia Lawall @ 2022-05-06  9:53 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: ksummit-discuss, linux-kernel, Nathan Chancellor, kbuild, lkp



On Fri, 6 May 2022, Dan Carpenter wrote:

> Ever since commit 78a5255ffb6a ("Stop the ad-hoc games with
> -Wno-maybe-initialized"), GCC's uninitialized variable warnings have
> been disabled by default.  Now, you have to turn on W=1 or W=2 to see
> the warnings which nobody except Arnd does.
>
> Disabling that has lead to a bunch of embarrassing bugs where variables
> are *never* initialized.  Very unsubtle bugs.  The bugs doesn't reach
> users because Nathan Chancellor and I review Clang and Smatch warnings
> respectively.  Also the kbuild-bot reports uninitialized variables.
>
> It's a lot to deal with.  Uninitialized variable bugs are probably the
> most common bug I have to deal with.
>
> It's frustrating.  Sometimes the false positives are hard to analyse
> because I have to read through multiple functions.  A lot of times
> when I write a patch and a commit message Nathan has already fixed it
> so it's just a waste of time.
>
> It's risky as well.  The Smatch check for uninitialized variables was
> broken for most of 2021.  Nathan sometimes goes on vacation.
>
> I guess I would hope that one day we can turn on the GCC uninitialized
> variable warnings again.  That would mean silencing false positives
> which a lot of people don't want to do...  Maybe Clang has fewer false
> positives than GCC?
>
> The Smatch check for uninitialized variable was deliberately written to
> be more strict than GCC because GCC was missing bugs.  So I think
> leaving Smatch false positives is fine.  There is a trade off between
> fewer false positives and missing bugs and Smatch is meant to err on the
> side of finding bugs but with the cost of false positives.
>
> Most of the Smatch uninitialized false positives are caused by loops:
>
> 	int i, ret;
>
> 	for (i = 0; i < bytes; i++) { // <-- what if bytes is zero?
> 		if (...)
> 			continue; // <-- can every iteration hit continue?
> 		ret = frob();
> 	}
>
> 	return ret;
>
> There is also stuff like this which is harmless:
>
> 	uint val;
>
> 	ret = read(&val);
> 	*p = val;  // <-- uninitialized variable if read() fails
> 	return ret;
>
> Btw, here is how to run Smatch on your code:
> https://staticthinking.wordpress.com/2022/04/25/how-to-run-smatch-on-your-code/

Could smatch inform the user that some results are likely false positives,
or even order the results according to their likely true positiveness?

julia

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Ksummit-discuss] uninitialized variables bugs
  2022-05-06  9:13 uninitialized variables bugs Dan Carpenter
  2022-05-06  9:53 ` [Ksummit-discuss] " Julia Lawall
@ 2022-05-06 11:56 ` Arnd Bergmann
  2022-05-06 16:23   ` Shuah Khan
  2022-05-06 21:50 ` Nathan Chancellor
  2022-05-22  9:07 ` [Ksummit-discuss] " Krzysztof Kozlowski
  3 siblings, 1 reply; 7+ messages in thread
From: Arnd Bergmann @ 2022-05-06 11:56 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: ksummit, Linux Kernel Mailing List, Nathan Chancellor, kbuild,
	kbuild test robot

On Fri, May 6, 2022 at 11:13 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:

>
> It's frustrating.  Sometimes the false positives are hard to analyse
> because I have to read through multiple functions.  A lot of times
> when I write a patch and a commit message Nathan has already fixed it
> so it's just a waste of time.

Agreed. I'm not actually checking for those warnings on gcc any more,
but just the clang warnings point to a bigger problem.

> It's risky as well.  The Smatch check for uninitialized variables was
> broken for most of 2021.  Nathan sometimes goes on vacation.
>
> I guess I would hope that one day we can turn on the GCC uninitialized
> variable warnings again.  That would mean silencing false positives
> which a lot of people don't want to do...  Maybe Clang has fewer false
> positives than GCC?

I think for the gcc warnings to become useful again, we may have to
wait for a future compiler release. I have not checked gcc-12 for this,
but it's a very old topic.

Fundamentally, it's impossible for any compiler to do this correctly,
because of the halting problem. gcc apparently has some heuristics
that worked well enough in the past, but it misses some obvious
cases and causes false positives in unexpected places, often
depending on optimization flags.

Recent gcc versions are much worse than older ones, since the
inlining changed in a way that caused a ton of false-positives.

clang is generally better at catching the simple cases reliably,
and it does this independent of optimization flags. However, it
stops at the function boundary, so it never catches some of the
cases that gcc was good at.

The gcc static analyzer apparently gained an option[1] that
works similarly to what you have in smatch. I have not tried
using this, but this may be something we can do in CI
systems that may not want to run smatch for some reason.

        Arnd

[1] https://gcc.gnu.org/onlinedocs/gcc-12.1.0/gcc/Static-Analyzer-Options.html#index-Wanalyzer-use-of-uninitialized-value

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Ksummit-discuss] uninitialized variables bugs
  2022-05-06 11:56 ` Arnd Bergmann
@ 2022-05-06 16:23   ` Shuah Khan
  0 siblings, 0 replies; 7+ messages in thread
From: Shuah Khan @ 2022-05-06 16:23 UTC (permalink / raw)
  To: Arnd Bergmann, Dan Carpenter
  Cc: Nathan Chancellor, kbuild, Linux Kernel Mailing List, ksummit,
	kbuild test robot, Shuah Khan

On 5/6/22 5:56 AM, Arnd Bergmann wrote:
> On Fri, May 6, 2022 at 11:13 AM Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
>>
>> It's frustrating.  Sometimes the false positives are hard to analyse
>> because I have to read through multiple functions.  A lot of times
>> when I write a patch and a commit message Nathan has already fixed it
>> so it's just a waste of time.
> 
> Agreed. I'm not actually checking for those warnings on gcc any more,
> but just the clang warnings point to a bigger problem.
> 
>> It's risky as well.  The Smatch check for uninitialized variables was
>> broken for most of 2021.  Nathan sometimes goes on vacation.
>>
>> I guess I would hope that one day we can turn on the GCC uninitialized
>> variable warnings again.  That would mean silencing false positives
>> which a lot of people don't want to do...  Maybe Clang has fewer false
>> positives than GCC?
> 

I would like to throw resource leak bugs in the mix. I am finding cppcheck
has been effective in finding them.

I am seeing a lot of file pointer leaks  in error legs in kselftest code error
paths. I have a few fixes in the works to send out.

We could discuss this topic at the LPC Kernel Testing and Dependability mini-conf
as well.

thanks,
-- Shuah




^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: uninitialized variables bugs
  2022-05-06  9:13 uninitialized variables bugs Dan Carpenter
  2022-05-06  9:53 ` [Ksummit-discuss] " Julia Lawall
  2022-05-06 11:56 ` Arnd Bergmann
@ 2022-05-06 21:50 ` Nathan Chancellor
  2022-05-08 12:17   ` Tom Rix
  2022-05-22  9:07 ` [Ksummit-discuss] " Krzysztof Kozlowski
  3 siblings, 1 reply; 7+ messages in thread
From: Nathan Chancellor @ 2022-05-06 21:50 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: ksummit-discuss, linux-kernel, kbuild, lkp, llvm,
	Nick Desaulniers, Tom Rix

Hi Dan,

On Fri, May 06, 2022 at 12:13:38PM +0300, Dan Carpenter wrote:
> Ever since commit 78a5255ffb6a ("Stop the ad-hoc games with
> -Wno-maybe-initialized"), GCC's uninitialized variable warnings have
> been disabled by default.  Now, you have to turn on W=1 or W=2 to see
> the warnings which nobody except Arnd does.

Thank you a lot for bringing this up; the situation does leave much to
be desired from my side, as I am having to fix quite a number of these
issues because people just are not seeing them and they break our builds
because of CONFIG_WERROR (as they should).

> Disabling that has lead to a bunch of embarrassing bugs where variables
> are *never* initialized.  Very unsubtle bugs.  The bugs doesn't reach
> users because Nathan Chancellor and I review Clang and Smatch warnings
> respectively.  Also the kbuild-bot reports uninitialized variables.

Thankfully, I believe the situation is a lot less worse than it could be
because the kbuild test robot tests with clang and finds these before
they make it into any tree:

https://lore.kernel.org/llvm/?q=f%3Alkp%40intel.com+Wuninitialized
https://lore.kernel.org/llvm/?q=f%3Alkp%40intel.com+Wsometimes-uninitialized

> It's a lot to deal with.  Uninitialized variable bugs are probably the
> most common bug I have to deal with.

Agreed.

> It's frustrating.  Sometimes the false positives are hard to analyse
> because I have to read through multiple functions.  A lot of times
> when I write a patch and a commit message Nathan has already fixed it
> so it's just a waste of time.

Sorry :( I should be better about either cc'ing you directly or adding
the kernel-janitors mailing list, as there are others who would benefit
from seeing these patches fly by. I know that isn't really the point of
the email but I'll try to make your life easier in the future.

> It's risky as well.  The Smatch check for uninitialized variables was
> broken for most of 2021.  Nathan sometimes goes on vacation.
> 
> I guess I would hope that one day we can turn on the GCC uninitialized
> variable warnings again.  That would mean silencing false positives
> which a lot of people don't want to do...  Maybe Clang has fewer false
> positives than GCC?

Yes, clang does have fewer false positives than GCC for a couple of
reasons:

1. As Arnd touched on, Clang's -Wuninitialized and
-Wsometimes-uninitialized do not check for initializations across
function boundaries. In your example below with 'read(&val)', clang will
assume that read() initializes val. While that does mean that there is
slightly less coverage, it does drives the false positive rate way down,
almost to zero. There are occasionally times where clang fails to figure
out certain conditions which will avoid an uninitialized use but I
believe that means the code is not as clear as it could be. For example,
commit 118de6106735 ("net: ethernet: rmnet: Restructure if checks to
avoid uninitialized warning").

2. clang used to only have these warnings under
-Wconditional-uninitialized, which suffers from the same issue as
-Wmaybe-uninitialized ("maybe it is uninitialized?").
-Wsometimes-uninitialized was split off from that warning back in 2011
to be more assertive ("this IS uninitialized if these conditions hold"):

https://github.com/llvm/llvm-project/commit/4323bf8e2e5135c49f814940b2b546298c01ecbc

Perhaps GCC could consider something to this?

Clang's static analyzer, which Tom regularly runs, will check variables
across function boundaries. I am not sure what the false positive rate
on that check is but it does turn up issues like smatch does.

> The Smatch check for uninitialized variable was deliberately written to
> be more strict than GCC because GCC was missing bugs.  So I think
> leaving Smatch false positives is fine.  There is a trade off between
> fewer false positives and missing bugs and Smatch is meant to err on the
> side of finding bugs but with the cost of false positives.

I would agree with this too.

Cheers,
Nathan

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: uninitialized variables bugs
  2022-05-06 21:50 ` Nathan Chancellor
@ 2022-05-08 12:17   ` Tom Rix
  0 siblings, 0 replies; 7+ messages in thread
From: Tom Rix @ 2022-05-08 12:17 UTC (permalink / raw)
  To: Nathan Chancellor, Dan Carpenter
  Cc: ksummit-discuss, linux-kernel, kbuild, lkp, llvm, Nick Desaulniers


On 5/6/22 2:50 PM, Nathan Chancellor wrote:
> Hi Dan,
>
> On Fri, May 06, 2022 at 12:13:38PM +0300, Dan Carpenter wrote:
>> Ever since commit 78a5255ffb6a ("Stop the ad-hoc games with
>> -Wno-maybe-initialized"), GCC's uninitialized variable warnings have
>> been disabled by default.  Now, you have to turn on W=1 or W=2 to see
>> the warnings which nobody except Arnd does.
...
> Clang's static analyzer, which Tom regularly runs, will check variables
> across function boundaries. I am not sure what the false positive rate
> on that check is but it does turn up issues like smatch does.

Clang's static analyzer is pretty go wrt uninitialized variables.

But the issues do not turn up in the report, the show up as errors and 
is why

I post a fix for a build break every couple of weeks.

Tom



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Ksummit-discuss] uninitialized variables bugs
  2022-05-06  9:13 uninitialized variables bugs Dan Carpenter
                   ` (2 preceding siblings ...)
  2022-05-06 21:50 ` Nathan Chancellor
@ 2022-05-22  9:07 ` Krzysztof Kozlowski
  3 siblings, 0 replies; 7+ messages in thread
From: Krzysztof Kozlowski @ 2022-05-22  9:07 UTC (permalink / raw)
  To: Dan Carpenter, ksummit-discuss, linux-kernel
  Cc: Nathan Chancellor, kbuild, lkp

On 06/05/2022 11:13, Dan Carpenter wrote:
> There is also stuff like this which is harmless:
> 
> 	uint val;
> 
> 	ret = read(&val);
> 	*p = val;  // <-- uninitialized variable if read() fails
> 	return ret;
> 
> Btw, here is how to run Smatch on your code:
> https://staticthinking.wordpress.com/2022/04/25/how-to-run-smatch-on-your-code/

In the topic of suppressing false positives we also have several
"fixes", sometimes pointed out incorrectly by Coverity, for missing
check for of_device_get_match_data().

Compare:
https://elixir.bootlin.com/linux/v5.18-rc7/source/drivers/clk/clk-aspeed.c#L415
https://elixir.bootlin.com/linux/v5.18-rc7/source/drivers/clk/clk-oxnas.c#L216

Although in theory the of_device_get_match_data() can return NULL, in
practice it is not possible because driver matches via OF thus there
will be always of_device_id->driver data.

Coverity screams about it, people fix it by adding checks for NULL,
which is pointless. Half of drivers add the !NULL check, half do not...

Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-05-22  9:08 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-06  9:13 uninitialized variables bugs Dan Carpenter
2022-05-06  9:53 ` [Ksummit-discuss] " Julia Lawall
2022-05-06 11:56 ` Arnd Bergmann
2022-05-06 16:23   ` Shuah Khan
2022-05-06 21:50 ` Nathan Chancellor
2022-05-08 12:17   ` Tom Rix
2022-05-22  9:07 ` [Ksummit-discuss] " Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).