linux-toolchains.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Nick Desaulniers <ndesaulniers@google.com>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Alexander Potapenko <glider@google.com>,
	Evgenii Stepanov <eugenis@google.com>,
	Kees Cook <keescook@chromium.org>, Marco Elver <elver@google.com>,
	Nathan Chancellor <nathan@kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Vitaly Buka <vitalybuka@google.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-toolchains <linux-toolchains@vger.kernel.org>
Subject: Re: [PATCH] [RFC] Initialization of unused function parameters
Date: Tue, 14 Jun 2022 10:11:37 -0700	[thread overview]
Message-ID: <CAKwvOd=SotrVcZshzGzsMprkORkVXFwYc-3mREkJSDCQ1nvbFw@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=whaWnwB8guceg8V=bA1adv74GNaMk2FEu+YQkBKUqxVoA@mail.gmail.com>

On Tue, Jun 14, 2022 at 9:48 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Tue, Jun 14, 2022 at 7:49 AM Alexander Potapenko <glider@google.com> wrote:
> >
> > The bigger question I want to raise here is whether we want to
> > discourage passing uninitialized variables to functions in the kernel
> > altogether.
>
> I'm assuming you mean pass by reference.
>
> Some functions are really fundamentally about initializing things, and
> expect uninitialized allocations.
>
> Obviously the traditional example of this is "memset()", and that one
> can be special-cased as obvious, but we probably have a ton of wrapper
> things like that.
>
> IOW, things like just "snprintf()" etc is fundamentally passed an
> uninitialized buffer, because the whole point is that it will write to
> that buffer.
>
> And no, we don't want to initialize it, since the buffer may be big
> (on purpose).
>
> Now, for *small* things (like that "pointer to inode") that aren't
> some kind of array or big structure, I think it might be good to
> perhaps be stricter. But even there we do have cases where we pass
> things by reference because the function is explicitly designed to
> initialize the value: the argument isn't really "an argument", it's a
> "second return value".
>
> But always initializing in the caller sounds stupid and
> counter-productive, since the point is to initialize by calling the
> helper function (think things like returning a "cookie" or similar:
> initializing the cookie to NULL in the caller is just plain _wrong_.
>
> What I think might be a good model is to be able to mark such
> arguments as "must be initialized by callee".

Yeah, being able to enforce that would be nice.

Now that we have clang's static analyzer wired up (commit 6ad7cbc01527
("Makefile: Add clang-tidy and static analyzer support to makefile")),
Intel's 0day bot has been reporting cases it finds for some classes of
warnings.  There's been a few interesting (to me) cases where these
"init" routines would conditionally initialize a "second return value"
but the caller either did not do return value checking or the callee
was not marked __must_check (or both).

As with -Wsometimes-uninitialized, my experience has been that folks
consistently get error handling/exceptional cases wrong in so far as
passing unitialized values later.  Clang's -Wsometimes-uninitialized
is intra-proceedural, so doesn't catch the problems with "init"
routines. Clang's static analyzer is interproceedural; the trade off
being the time the analysis takes.

Maybe a new function parameter attribute would be nice?

#define __must_init __attribute__((must_init))
int init (int * __must_init x) {
// ^ warning: function parameter x marked '__attribute__((must_init))'
not unconditionally initialized
  if (stars_dont_align) {
    return -42;
  }
  *x = 42;
  return 0;
}
void foo (void) {  int x; init(&x); /* use of x without fear */ }


>
> So then the rule could be that small arguments passed by reference
> have to be either initialized by the caller, or the argument must have
> that "initialized by callee" attribute, and then the initialization
> would be enforced in the callee instead.
>
> But making the rule be that the caller *always* has to initialize
> sounds really wrong to me.
>
>              Linus



-- 
Thanks,
~Nick Desaulniers

  reply	other threads:[~2022-06-14 17:12 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-06-14 14:48 [PATCH] [RFC] Initialization of unused function parameters Alexander Potapenko
2022-06-14 16:48 ` Linus Torvalds
2022-06-14 17:11   ` Nick Desaulniers [this message]
2022-06-14 17:24     ` Linus Torvalds
2022-06-14 18:08       ` Nick Desaulniers
2022-06-14 22:27         ` Peter Zijlstra
2022-06-14 18:07   ` Alexander Potapenko
2022-06-14 18:30     ` Linus Torvalds
2022-06-14 20:19       ` Alexander Potapenko
2022-06-14 20:43         ` Linus Torvalds
2022-06-14 21:40         ` Segher Boessenkool
2022-06-14 22:08           ` Evgenii Stepanov
2022-06-15  8:30           ` Alexander Potapenko
2022-06-15 16:46             ` Segher Boessenkool

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='CAKwvOd=SotrVcZshzGzsMprkORkVXFwYc-3mREkJSDCQ1nvbFw@mail.gmail.com' \
    --to=ndesaulniers@google.com \
    --cc=elver@google.com \
    --cc=eugenis@google.com \
    --cc=glider@google.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-toolchains@vger.kernel.org \
    --cc=nathan@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=vitalybuka@google.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).