{"thread":{"id":"64757","subject":"[Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt","startedAt":"2026-01-09T01:25:06Z","lastAt":"2026-01-11T14:20:52Z","messageCount":7,"participants":["correctmost","Patrick Steinhardt","Adrian Ratiu","Ben Knoble","brian m. carlson"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"533307","messageId":"72d123b8-b75e-4b1d-8506-95eb9ad350da@app.fastmail.com","threadId":"64757","inReplyTo":null,"subject":"[Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt","fromName":"correctmost","fromEmail":"cmlists@sent.com","sentAt":"2026-01-09T01:24:01Z","receivedAt":"2026-01-09T01:25:06Z","isPatch":false,"sender":{"key":"cmlists@sent.com","avatar":null},"body":"Hi,\n\nGCC 15.2.1 warns about a potential NULL pointer dereference in run_hooks_opt on the master branch:\n\n---\n\n../hook.c: In function ‘run_hooks_opt’:\n../hook.c:167:12: error: check of ‘options’ for NULL after already dereferencing it [-Werror=analyzer-deref-before-check]\n  167 |         if (!options)\n      |            ^\n\n[...snip...]\n\n    │  156 |                 .ungroup = options->ungroup,\n    │      |                            ~~~~~~~~~~~~~~~~\n    │      |                                   |\n    │      |                                   (7) pointer ‘options’ is dereferenced here\n    │......\n    │  167 |         if (!options)\n    │      |            ~                           \n    │      |            |\n    │      |            (8)   pointer ‘options’ is checked for NULL here but it was already dereferenced at (7)\n    │\n\n---\n\nThis 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.\").\n\nI noticed the warning while compiling commit d529f3a19736 on Arch Linux.\n\nThanks!\n"},{"id":"533336","messageId":"aWDm_n2YgjvaRmpV@pks.im","threadId":"64757","inReplyTo":"72d123b8-b75e-4b1d-8506-95eb9ad350da@app.fastmail.com","subject":"Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-09T11:31:10Z","receivedAt":"2026-01-09T11:31:16Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 08, 2026 at 08:24:01PM -0500, correctmost wrote:\n> Hi,\n> \n> GCC 15.2.1 warns about a potential NULL pointer dereference in\n> run_hooks_opt on the master branch:\n> \n> ---\n> \n> ../hook.c: In function ‘run_hooks_opt’:\n> ../hook.c:167:12: error: check of ‘options’ for NULL after already dereferencing it [-Werror=analyzer-deref-before-check]\n>   167 |         if (!options)\n>       |            ^\n> \n> [...snip...]\n> \n>     │  156 |                 .ungroup = options->ungroup,\n>     │      |                            ~~~~~~~~~~~~~~~~\n>     │      |                                   |\n>     │      |                                   (7) pointer ‘options’ is dereferenced here\n>     │......\n>     │  167 |         if (!options)\n>     │      |            ~                           \n>     │      |            |\n>     │      |            (8)   pointer ‘options’ is checked for NULL here but it was already dereferenced at (7)\n>     │\n> \n> ---\n> \n> This does seem like a real bug, though I'm not sure how likely it is\n> to occur.  It looks like the warning was introduced in merge commit\n> f406b89552 (\"Use hook API to replace ad-hoc invocation of hook scripts\n> with the run_command() API.\").\n> \n> I noticed the warning while compiling commit d529f3a19736 on Arch\n> Linux.\n\nIt's not a real bug. If you take a look at the the `if (!options)`\ncheck, you'll see:\n\n\tif (!options)\n\t\tBUG(\"a struct run_hooks_opt must be provided to run_hooks\");\n\nSo we'd abort immediatly with an error message in case the pointer was\n`NULL`. Which clarifies that this is a case that shouldn't ever happen\nin the first place.\n\nThat being said, it's of course a bit careless to dereference the\npointer before we have the opportunity to call `BUG()`. I see two ways\nto fix this:\n\n  - We can either move all derefs of `options` after the call to\n    `BUG()`.\n\n  - Or we can drop the call to `BUG()` altogether.\n\nOut of those two I think I slightly lean towards the latter, mostly\nbecause the resulting code structure is simpler. And we'd reliably\nsegfault anyway if we dereference the pointer, even though we would not\nget a clean error message. Not sure whether that really is worth the\nhassle though.\n\nCc'ing Adrian, the author of this.\n\nThanks!\n\nPatrick\n"},{"id":"533340","messageId":"87jyxrvucx.fsf@gentoo.mail-host-address-is-not-set","threadId":"64757","inReplyTo":"aWDm_n2YgjvaRmpV@pks.im","subject":"Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-09T12:33:18Z","receivedAt":"2026-01-09T12:33:31Z","isPatch":false,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Fri, 09 Jan 2026, Patrick Steinhardt <ps@pks.im> wrote:\n> On Thu, Jan 08, 2026 at 08:24:01PM -0500, correctmost wrote:\n>> Hi,\n>> \n>> GCC 15.2.1 warns about a potential NULL pointer dereference in\n>> run_hooks_opt on the master branch:\n>> \n>> ---\n>> \n>> ../hook.c: In function ‘run_hooks_opt’:\n>> ../hook.c:167:12: error: check of ‘options’ for NULL after already dereferencing it [-Werror=analyzer-deref-before-check]\n>>   167 |         if (!options)\n>>       |            ^\n>> \n>> [...snip...]\n>> \n>>     │  156 |                 .ungroup = options->ungroup,\n>>     │      |                            ~~~~~~~~~~~~~~~~\n>>     │      |                                   |\n>>     │      |                                   (7) pointer ‘options’ is dereferenced here\n>>     │......\n>>     │  167 |         if (!options)\n>>     │      |            ~                           \n>>     │      |            |\n>>     │      |            (8)   pointer ‘options’ is checked for NULL here but it was already dereferenced at (7)\n>>     │\n>> \n>> ---\n>> \n>> This does seem like a real bug, though I'm not sure how likely it is\n>> to occur.  It looks like the warning was introduced in merge commit\n>> f406b89552 (\"Use hook API to replace ad-hoc invocation of hook scripts\n>> with the run_command() API.\").\n>> \n>> I noticed the warning while compiling commit d529f3a19736 on Arch\n>> Linux.\n>\n> It's not a real bug. If you take a look at the the `if (!options)`\n> check, you'll see:\n>\n> \tif (!options)\n> \t\tBUG(\"a struct run_hooks_opt must be provided to run_hooks\");\n>\n> So we'd abort immediatly with an error message in case the pointer was\n> `NULL`. Which clarifies that this is a case that shouldn't ever happen\n> in the first place.\n>\n> That being said, it's of course a bit careless to dereference the\n> pointer before we have the opportunity to call `BUG()`. I see two ways\n> to fix this:\n>\n>   - We can either move all derefs of `options` after the call to\n>     `BUG()`.\n>\n>   - Or we can drop the call to `BUG()` altogether.\n>\n> Out of those two I think I slightly lean towards the latter, mostly\n> because the resulting code structure is simpler. And we'd reliably\n> segfault anyway if we dereference the pointer, even though we would not\n> get a clean error message. Not sure whether that really is worth the\n> hassle though.\n\nYour diagnosis is correct: options is never NULL in practice.\n\nI'd like to keep that BUG() and move it before dereferencing, just in\ncase some future code change accidentally calls run_hooks_opt() with a\nNULL options, so we get a clean error and not trigger the compiler check. \n\nWill see if I can make the code structure nice and send a patch.\n"},{"id":"533385","messageId":"7BD989A0-30B1-418E-9257-1731724DCB72@gmail.com","threadId":"64757","inReplyTo":"aWDm_n2YgjvaRmpV@pks.im","subject":"Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt","fromName":"Ben Knoble","fromEmail":"ben.knoble@gmail.com","sentAt":"2026-01-09T16:02:40Z","receivedAt":"2026-01-09T16:02:52Z","isPatch":false,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"\n> Le 9 janv. 2026 à 06:31, Patrick Steinhardt <ps@pks.im> a écrit :\n\n[snip]\n\n> And we'd reliably\n> segfault anyway if we dereference the pointer, even though we would not\n> get a clean error message. Not sure whether that really is worth the\n> hassle though.\n\nI 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 :)\n\nBest,\nBen"},{"id":"533386","messageId":"aWEqbqPRq5Ie9XTo@pks.im","threadId":"64757","inReplyTo":"7BD989A0-30B1-418E-9257-1731724DCB72@gmail.com","subject":"Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-09T16:18:54Z","receivedAt":"2026-01-09T16:19:00Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jan 09, 2026 at 11:02:40AM -0500, Ben Knoble wrote:\n> \n> > Le 9 janv. 2026 à 06:31, Patrick Steinhardt <ps@pks.im> a écrit :\n> \n> [snip]\n> \n> > And we'd reliably\n> > segfault anyway if we dereference the pointer, even though we would not\n> > get a clean error message. Not sure whether that really is worth the\n> > hassle though.\n> \n> 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 :)\n\nIt is, yeah. But I'd claim that on almost all platforms out there it\nwould segfault anyway.\n\nPatrick\n"},{"id":"533448","messageId":"aWF-nZ9MXp31QzXs@fruit.crustytoothpaste.net","threadId":"64757","inReplyTo":"aWDm_n2YgjvaRmpV@pks.im","subject":"Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2026-01-09T22:18:05Z","receivedAt":"2026-01-09T22:18:07Z","isPatch":false,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2026-01-09 at 11:31:10, Patrick Steinhardt wrote:\n> It's not a real bug. If you take a look at the the `if (!options)`\n> check, you'll see:\n> \n> \tif (!options)\n> \t\tBUG(\"a struct run_hooks_opt must be provided to run_hooks\");\n> \n> So we'd abort immediatly with an error message in case the pointer was\n> `NULL`. Which clarifies that this is a case that shouldn't ever happen\n> in the first place.\n\nYou might think that we'd abort, but that's not what modern compilers\ndo. Dereferencing `options` if it is NULL is undefined behaviour.\nCompilers are free to assume that undefined behaviour never happens, so\nwhat most modern compilers do is say, \"Oh, we've dereferenced `options`,\nso it can never be NULL,\" and then use that to omit the check\naltogether.\n\nThis sounds bizarre and like it might actually lead to security bugs,\nand you're right.  However, compilers keep wanting to make code go\nfaster, so they keep relying on eliminating undefined behaviour to make\nmore assumptions about the code to optimize it, even if that results in\ncode that doesn't do what the programmer intended.\n\nThis is one of the reasons why I'm in favour of writing more Rust, since\nsafe Rust doesn't have undefined behaviour and therefore doesn't suffer\nfrom these problems.\n\nIn any event, this is almost certainly a bug because it almost certainly\ndoes not do what it looks like it does and the compiler is right to warn\nabout it.\n-- \nbrian m. carlson (they/them)\nToronto, Ontario, CA\n"},{"id":"533526","messageId":"87bjj0s023.fsf@collabora.com","threadId":"64757","inReplyTo":"aWF-nZ9MXp31QzXs@fruit.crustytoothpaste.net","subject":"Re: [Bug] hook: -Wanalyzer-deref-before-check warning in run_hooks_opt","fromName":"Adrian Ratiu","fromEmail":"adrian.ratiu@collabora.com","sentAt":"2026-01-11T14:20:36Z","receivedAt":"2026-01-11T14:20:52Z","isPatch":false,"sender":{"key":"adrian.ratiu@collabora.com","avatar":"https://avatars.githubusercontent.com/u/12472556?v=4"},"body":"On Fri, 09 Jan 2026, \"brian m. carlson\" <sandals@crustytoothpaste.net> wrote:\n> On 2026-01-09 at 11:31:10, Patrick Steinhardt wrote:\n>> It's not a real bug. If you take a look at the the `if (!options)`\n>> check, you'll see:\n>> \n>> \tif (!options)\n>> \t\tBUG(\"a struct run_hooks_opt must be provided to run_hooks\");\n>> \n>> So we'd abort immediatly with an error message in case the pointer was\n>> `NULL`. Which clarifies that this is a case that shouldn't ever happen\n>> in the first place.\n>\n> You might think that we'd abort, but that's not what modern compilers\n> do. Dereferencing `options` if it is NULL is undefined behaviour.\n> Compilers are free to assume that undefined behaviour never happens, so\n> what most modern compilers do is say, \"Oh, we've dereferenced `options`,\n> so it can never be NULL,\" and then use that to omit the check\n> altogether.\n>\n> This sounds bizarre and like it might actually lead to security bugs,\n> and you're right.  However, compilers keep wanting to make code go\n> faster, so they keep relying on eliminating undefined behaviour to make\n> more assumptions about the code to optimize it, even if that results in\n> code that doesn't do what the programmer intended.\n>\n> This is one of the reasons why I'm in favour of writing more Rust, since\n> safe Rust doesn't have undefined behaviour and therefore doesn't suffer\n> from these problems.\n>\n> In any event, this is almost certainly a bug because it almost certainly\n> does not do what it looks like it does and the compiler is right to warn\n> about it.\n\nAck and thanks for the feedback. 100% agreed on writing more Rust. :)\n\nSee the below link for the fix. I've ensured the NULL check happens before\ndereferencing (many thanks to Patrick as well).\n\nhttps://lore.kernel.org/git/87ecnws0fx.fsf@gentoo.mail-host-address-is-not-set/T/#ma8343d1b5393d4efc0c1103357a6e684fc8b1017\n"}]}