ksummit.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Dan Carpenter <dan.carpenter@oracle.com>
Subject: [Ksummit-discuss] uninitialized variables bugs
Date: Fri, 6 May 2022 12:13:38 +0300	[thread overview]
Message-ID: <20220506091338.GE4031@kadam> (raw)

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


             reply	other threads:[~2022-05-06  9:13 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-06  9:13 Dan Carpenter [this message]
2022-05-06  9:53 ` [Ksummit-discuss] uninitialized variables bugs Julia Lawall
2022-05-06 11:56 ` Arnd Bergmann
2022-05-06 16:23   ` Shuah Khan
2022-05-22  9:07 ` Krzysztof Kozlowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220506091338.GE4031@kadam \
    --to=dan.carpenter@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).