Re: [nft PATCH] evaluate: Reject set references in mapping LHS

[Phil Sutter <phil@nwl.cc>]

On Thu, Nov 14, 2019 at 12:10:22AM +0100, Pablo Neira Ayuso wrote:
> On Wed, Nov 13, 2019 at 11:10:50AM +0100, Phil Sutter wrote:
> > On Tue, Nov 12, 2019 at 11:18:27PM +0100, Phil Sutter wrote:
> > > On Tue, Nov 12, 2019 at 10:45:18PM +0100, Pablo Neira Ayuso wrote:
> > > > On Thu, Oct 31, 2019 at 07:21:24PM +0100, Phil Sutter wrote:
> > > > > This wasn't explicitly caught before causing a program abort:
> > > > > 
> > > > > | BUG: invalid range expression type set reference
> > > > > | nft: expression.c:1162: range_expr_value_low: Assertion `0' failed.
> > > > > | zsh: abort      sudo ./install/sbin/nft add rule t c meta mark set tcp dport map '{ @s : 23 }
> > > > > 
> > > > > With this patch in place, the error message is way more descriptive:
> > > > > 
> > > > > | Error: Key can't be set reference
> > > > > | add rule t c meta mark set tcp dport map { @s : 23 }
> > > > > |                                            ^^
> > > > 
> > > > I wanted to check why the parser allow for this...
> > > 
> > > For set elements or LHS parts of map elements, there is set_lhs_expr.
> > > The latter may be concat_rhs_expr or multiton_rhs_expr. concat_rhs_expr
> > > eventually resolves into primary_rhs_expr which may be symbol_expr.
> > > 
> > > BTW, it seems like from parser side, set references on map element's
> > > RHS are allowed as well.
> > > 
> > > IMHO, parser_bison.y slowly but steadily turns into a can of worms. :(
> > 
> > On a second look, I start wondering whether symbol_expr was a wise
> > choice: This thing combines variables ('$' identifier), "unidentified"
> > strings and set references (AT identifier).
> > 
> > With symbol_expr being listed in both primary_expr and primary_rhs_expr,
> > set references are allowed about anywhere - even in concatenations or
> > any binary operation.
> 
> It would be probably good to restrict set references to where it makes
> sense only. This is good for the grammar and we don't need to validate
> all possible invalid combinations from the evaluation step.

ACK!

> Would you have a look or you think it's too complicated to attack this
> from the parser?

It's not too complicated, but I sometimes feel like turning adjuster
screws on a machine I don't understand. OK, given that parser_bison.y
was initially written by Patrick, you're probably in the same situation.
:)

Cheers, Phil