| Message ID | 20191031182124.11393-1-phil@nwl.cc |
|---|---|
| State | Superseded |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nft] evaluate: Reject set references in mapping LHS | expand |
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... > Signed-off-by: Phil Sutter <phil@nwl.cc> > --- > src/evaluate.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/src/evaluate.c b/src/evaluate.c > index 81230fc7f4be4..500780aeae243 100644 > --- a/src/evaluate.c > +++ b/src/evaluate.c > @@ -1456,6 +1456,10 @@ static int expr_evaluate_mapping(struct eval_ctx *ctx, struct expr **expr) > if (!expr_is_constant(mapping->left)) > return expr_error(ctx->msgs, mapping->left, > "Key must be a constant"); > + if (mapping->left->etype == EXPR_SET_ELEM && > + mapping->left->key->etype == EXPR_SET_REF) > + return expr_error(ctx->msgs, mapping->left, > + "Key can't be set reference"); > mapping->flags |= mapping->left->flags & EXPR_F_SINGLETON; > > expr_set_context(&ctx->ectx, set->datatype, set->datalen); > -- > 2.23.0 >
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. :( Cheers, Phil
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. Cheers, Phil
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. Would you have a look or you think it's too complicated to attack this from the parser?
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
diff --git a/src/evaluate.c b/src/evaluate.c index 81230fc7f4be4..500780aeae243 100644 --- a/src/evaluate.c +++ b/src/evaluate.c @@ -1456,6 +1456,10 @@ static int expr_evaluate_mapping(struct eval_ctx *ctx, struct expr **expr) if (!expr_is_constant(mapping->left)) return expr_error(ctx->msgs, mapping->left, "Key must be a constant"); + if (mapping->left->etype == EXPR_SET_ELEM && + mapping->left->key->etype == EXPR_SET_REF) + return expr_error(ctx->msgs, mapping->left, + "Key can't be set reference"); mapping->flags |= mapping->left->flags & EXPR_F_SINGLETON; expr_set_context(&ctx->ectx, set->datatype, set->datalen);
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 } | ^^ Signed-off-by: Phil Sutter <phil@nwl.cc> --- src/evaluate.c | 4 ++++ 1 file changed, 4 insertions(+)