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

> 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.

> 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.

> 
> > 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'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.

> 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.