Re: [PATCH nft v3 1/2] json: drop handling missing json() hook in expr_print_json()

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Sun, Nov 05, 2023 at 11:40:22AM +0100, Phil Sutter wrote:
> On Sat, Nov 04, 2023 at 06:28:30AM +0100, Thomas Haller wrote:
> > On Fri, 2023-11-03 at 22:37 +0100, Phil Sutter wrote:
> > > On Fri, Nov 03, 2023 at 05:25:13PM +0100, Thomas Haller wrote:
> > > [...]
> > > > +	/* The json() hooks of "symbol_expr_ops" and
> > > > "variable_expr_ops" are
> > > > +	 * known to be NULL, but for such expressions we never
> > > > expect to call
> > > > +	 * expr_print_json().
> > > > +	 *
> > > > +	 * All other expr_ops must have a json() hook.
> > > > +	 *
> > > > +	 * Unconditionally access the hook (and segfault in case
> > > > of a bug).  */
> > > > +	return expr_ops(expr)->json(expr, octx);
> > > 
> > > This does not make sense to me. You're deliberately dropping any
> > > error
> > > handling
> > 
> > Error handling for what is clearly a bug. Don't try to handle bugs.
> > Avoid bugs and fix them.
> 
> Yeah, indeed. Let's go ahead and drop all BUG() statements as well.
> Seriously, I doubt nftables users agree the software should segfault
> instead of aborting with an error message.

BUG() assertion is better than crash.

> > > and accept a segfault because "it should never happen"? All it
> > > takes is someone to add a new expression type and forget about the
> > > JSON
> > > API.
> > 
> > There will be a unit test guarding against that, once the unit test
> > basics are done.
> > 
> > Also, if you "forget" to implement the JSON hook and test it (manually)
> > only a single time, then you'll notice right away.
> 
> Actually, all it takes to notice things don't add up is running the py
> testsuite with '-j' arg after adding the obligatory "unit" tests there.
> Feel free to search the git history for late additions of .json
> callbacks. I think the message is pretty clear.
> 
> > > If you absolutely have to remove that fallback code, at least add a
> > > BUG() explaining the situation. The sysadmin looking at the segfault
> > > report in syslog won't see your comment above.
> > 
> > I am in favor of adding assertions all over the place. The project
> > doesn't use enough asserions for my taste.
> > 
> > In this case, it seems hard to mess up the condition, and you get a
> > very clear signal when you do (segfault). That makes the assert()/BUG()
> > kinda unecessary.
> 
> The clear signal being "oops, my program crashed" when it could be a
> dubious "oops, there is no JSON callback for this expression type".

This should be turned into BUG().