[Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt

[Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt

[correctmost]

Hi,

GCC 15.2.1 warns about a potential NULL pointer dereference in run_hooks_opt on the master branch:

---

../hook.c: In function ‘run_hooks_opt’:
../hook.c:167:12: error: check of ‘options’ for NULL after already dereferencing it [-Werror=analyzer-deref-before-check]
  167 |         if (!options)
      |            ^

[...snip...]

    │  156 |                 .ungroup = options->ungroup,
    │      |                            ~~~~~~~~~~~~~~~~
    │      |                                   |
    │      |                                   (7) pointer ‘options’ is dereferenced here
    │......
    │  167 |         if (!options)
    │      |            ~                           
    │      |            |
    │      |            (8)   pointer ‘options’ is checked for NULL here but it was already dereferenced at (7)
    │

---

This does seem like a real bug, though I'm not sure how likely it is to occur.  It looks like the warning was introduced in merge commit f406b89552 ("Use hook API to replace ad-hoc invocation of hook scripts with the run_command() API.").

I noticed the warning while compiling commit d529f3a19736 on Arch Linux.

Thanks!

Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt

[Patrick Steinhardt]

On Thu, Jan 08, 2026 at 08:24:01PM -0500, correctmost wrote:
> Hi,
> 
> GCC 15.2.1 warns about a potential NULL pointer dereference in
> run_hooks_opt on the master branch:
> 
> ---
> 
> ../hook.c: In function ‘run_hooks_opt’:
> ../hook.c:167:12: error: check of ‘options’ for NULL after already dereferencing it [-Werror=analyzer-deref-before-check]
>   167 |         if (!options)
>       |            ^
> 
> [...snip...]
> 
>     │  156 |                 .ungroup = options->ungroup,
>     │      |                            ~~~~~~~~~~~~~~~~
>     │      |                                   |
>     │      |                                   (7) pointer ‘options’ is dereferenced here
>     │......
>     │  167 |         if (!options)
>     │      |            ~                           
>     │      |            |
>     │      |            (8)   pointer ‘options’ is checked for NULL here but it was already dereferenced at (7)
>     │
> 
> ---
> 
> This does seem like a real bug, though I'm not sure how likely it is
> to occur.  It looks like the warning was introduced in merge commit
> f406b89552 ("Use hook API to replace ad-hoc invocation of hook scripts
> with the run_command() API.").
> 
> I noticed the warning while compiling commit d529f3a19736 on Arch
> Linux.

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.

That being said, it's of course a bit careless to dereference the
pointer before we have the opportunity to call `BUG()`. I see two ways
to fix this:

  - We can either move all derefs of `options` after the call to
    `BUG()`.

  - Or we can drop the call to `BUG()` altogether.

Out of those two I think I slightly lean towards the latter, mostly
because the resulting code structure is simpler. And we'd reliably
segfault anyway if we dereference the pointer, even though we would not
get a clean error message. Not sure whether that really is worth the
hassle though.

Cc'ing Adrian, the author of this.

Thanks!

Patrick

Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt

[Adrian Ratiu]

On Fri, 09 Jan 2026, Patrick Steinhardt <ps@pks.im> wrote:
> On Thu, Jan 08, 2026 at 08:24:01PM -0500, correctmost wrote:
>> Hi,
>> 
>> GCC 15.2.1 warns about a potential NULL pointer dereference in
>> run_hooks_opt on the master branch:
>> 
>> ---
>> 
>> ../hook.c: In function ‘run_hooks_opt’:
>> ../hook.c:167:12: error: check of ‘options’ for NULL after already dereferencing it [-Werror=analyzer-deref-before-check]
>>   167 |         if (!options)
>>       |            ^
>> 
>> [...snip...]
>> 
>>     │  156 |                 .ungroup = options->ungroup,
>>     │      |                            ~~~~~~~~~~~~~~~~
>>     │      |                                   |
>>     │      |                                   (7) pointer ‘options’ is dereferenced here
>>     │......
>>     │  167 |         if (!options)
>>     │      |            ~                           
>>     │      |            |
>>     │      |            (8)   pointer ‘options’ is checked for NULL here but it was already dereferenced at (7)
>>     │
>> 
>> ---
>> 
>> This does seem like a real bug, though I'm not sure how likely it is
>> to occur.  It looks like the warning was introduced in merge commit
>> f406b89552 ("Use hook API to replace ad-hoc invocation of hook scripts
>> with the run_command() API.").
>> 
>> I noticed the warning while compiling commit d529f3a19736 on Arch
>> Linux.
>
> 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.
>
> That being said, it's of course a bit careless to dereference the
> pointer before we have the opportunity to call `BUG()`. I see two ways
> to fix this:
>
>   - We can either move all derefs of `options` after the call to
>     `BUG()`.
>
>   - Or we can drop the call to `BUG()` altogether.
>
> Out of those two I think I slightly lean towards the latter, mostly
> because the resulting code structure is simpler. And we'd reliably
> segfault anyway if we dereference the pointer, even though we would not
> get a clean error message. Not sure whether that really is worth the
> hassle though.

Your diagnosis is correct: options is never NULL in practice.

I'd like to keep that BUG() and move it before dereferencing, just in
case some future code change accidentally calls run_hooks_opt() with a
NULL options, so we get a clean error and not trigger the compiler check. 

Will see if I can make the code structure nice and send a patch.

Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt

[Ben Knoble]

> Le 9 janv. 2026 à 06:31, Patrick Steinhardt <ps@pks.im> a écrit :

[snip]

> And we'd reliably
> segfault anyway if we dereference the pointer, even though we would not
> get a clean error message. Not sure whether that really is worth the
> hassle though.

I think you’re probably right in practice, but doesn’t the standard just say dereferencing a NULL pointer is undefined behavior? Just wondering for my own curiosity :)

Best,
Ben

Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt

[Patrick Steinhardt]

On Fri, Jan 09, 2026 at 11:02:40AM -0500, Ben Knoble wrote:
> 
> > Le 9 janv. 2026 à 06:31, Patrick Steinhardt <ps@pks.im> a écrit :
> 
> [snip]
> 
> > And we'd reliably
> > segfault anyway if we dereference the pointer, even though we would not
> > get a clean error message. Not sure whether that really is worth the
> > hassle though.
> 
> I think you’re probably right in practice, but doesn’t the standard just say dereferencing a NULL pointer is undefined behavior? Just wondering for my own curiosity :)

It is, yeah. But I'd claim that on almost all platforms out there it
would segfault anyway.

Patrick

Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1500 bytes --]

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.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt

[Adrian Ratiu]

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