All of lore.kernel.org
 help / color / mirror / Atom feed
From: Adrian Ratiu <adrian.ratiu@collabora.com>
To: "brian m. carlson" <sandals@crustytoothpaste.net>,
	Patrick Steinhardt <ps@pks.im>
Cc: correctmost <cmlists@sent.com>, git@vger.kernel.org
Subject: Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt
Date: Sun, 11 Jan 2026 16:20:36 +0200	[thread overview]
Message-ID: <87bjj0s023.fsf@collabora.com> (raw)
In-Reply-To: <aWF-nZ9MXp31QzXs@fruit.crustytoothpaste.net>

On Fri, 09 Jan 2026, "brian m. carlson" <sandals@crustytoothpaste.net> wrote:
> On 2026-01-09 at 11:31:10, Patrick Steinhardt wrote:
>> It's not a real bug. If you take a look at the the `if (!options)`
>> check, you'll see:
>> 
>> 	if (!options)
>> 		BUG("a struct run_hooks_opt must be provided to run_hooks");
>> 
>> So we'd abort immediatly with an error message in case the pointer was
>> `NULL`. Which clarifies that this is a case that shouldn't ever happen
>> in the first place.
>
> You might think that we'd abort, but that's not what modern compilers
> do. Dereferencing `options` if it is NULL is undefined behaviour.
> Compilers are free to assume that undefined behaviour never happens, so
> what most modern compilers do is say, "Oh, we've dereferenced `options`,
> so it can never be NULL," and then use that to omit the check
> altogether.
>
> This sounds bizarre and like it might actually lead to security bugs,
> and you're right.  However, compilers keep wanting to make code go
> faster, so they keep relying on eliminating undefined behaviour to make
> more assumptions about the code to optimize it, even if that results in
> code that doesn't do what the programmer intended.
>
> This is one of the reasons why I'm in favour of writing more Rust, since
> safe Rust doesn't have undefined behaviour and therefore doesn't suffer
> from these problems.
>
> In any event, this is almost certainly a bug because it almost certainly
> does not do what it looks like it does and the compiler is right to warn
> about it.

Ack and thanks for the feedback. 100% agreed on writing more Rust. :)

See the below link for the fix. I've ensured the NULL check happens before
dereferencing (many thanks to Patrick as well).

https://lore.kernel.org/git/87ecnws0fx.fsf@gentoo.mail-host-address-is-not-set/T/#ma8343d1b5393d4efc0c1103357a6e684fc8b1017

      reply	other threads:[~2026-01-11 14:20 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-01-09  1:24 [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt correctmost
2026-01-09 11:31 ` Patrick Steinhardt
2026-01-09 12:33   ` Adrian Ratiu
2026-01-09 16:02   ` Ben Knoble
2026-01-09 16:18     ` Patrick Steinhardt
2026-01-09 22:18   ` brian m. carlson
2026-01-11 14:20     ` Adrian Ratiu [this message]

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=87bjj0s023.fsf@collabora.com \
    --to=adrian.ratiu@collabora.com \
    --cc=cmlists@sent.com \
    --cc=git@vger.kernel.org \
    --cc=ps@pks.im \
    --cc=sandals@crustytoothpaste.net \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.