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

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 9, 2026, 11:31 UTC
Message-ID: <aWDm_n2YgjvaRmpV@pks.im>
In-Reply-To: <72d123b8-b75e-4b1d-8506-95eb9ad350da@app.fastmail.com>

On Thu, Jan 08, 2026 at 08:24:01PM -0500, correctmost wrote:

Show 34 quoted lines

> 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