* [nft PATCH] evaluate: Reject set references in mapping LHS
@ 2019-10-31 18:21 Phil Sutter
2019-11-12 21:45 ` Pablo Neira Ayuso
0 siblings, 1 reply; 6+ messages in thread
From: Phil Sutter @ 2019-10-31 18:21 UTC (permalink / raw)
To: Pablo Neira Ayuso; +Cc: netfilter-devel
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(+)
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [nft PATCH] evaluate: Reject set references in mapping LHS 2019-10-31 18:21 [nft PATCH] evaluate: Reject set references in mapping LHS Phil Sutter @ 2019-11-12 21:45 ` Pablo Neira Ayuso 2019-11-12 22:18 ` Phil Sutter 0 siblings, 1 reply; 6+ messages in thread From: Pablo Neira Ayuso @ 2019-11-12 21:45 UTC (permalink / raw) To: Phil Sutter; +Cc: netfilter-devel 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 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [nft PATCH] evaluate: Reject set references in mapping LHS 2019-11-12 21:45 ` Pablo Neira Ayuso @ 2019-11-12 22:18 ` Phil Sutter 2019-11-13 10:10 ` Phil Sutter 0 siblings, 1 reply; 6+ messages in thread From: Phil Sutter @ 2019-11-12 22:18 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [nft PATCH] evaluate: Reject set references in mapping LHS 2019-11-12 22:18 ` Phil Sutter @ 2019-11-13 10:10 ` Phil Sutter 2019-11-13 23:10 ` Pablo Neira Ayuso 0 siblings, 1 reply; 6+ messages in thread From: Phil Sutter @ 2019-11-13 10:10 UTC (permalink / raw) To: Pablo Neira Ayuso, netfilter-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [nft PATCH] evaluate: Reject set references in mapping LHS 2019-11-13 10:10 ` Phil Sutter @ 2019-11-13 23:10 ` Pablo Neira Ayuso 2019-11-14 10:52 ` Phil Sutter 0 siblings, 1 reply; 6+ messages in thread From: Pablo Neira Ayuso @ 2019-11-13 23:10 UTC (permalink / raw) To: Phil Sutter, netfilter-devel 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? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [nft PATCH] evaluate: Reject set references in mapping LHS 2019-11-13 23:10 ` Pablo Neira Ayuso @ 2019-11-14 10:52 ` Phil Sutter 0 siblings, 0 replies; 6+ messages in thread From: Phil Sutter @ 2019-11-14 10:52 UTC (permalink / raw) To: Pablo Neira Ayuso; +Cc: netfilter-devel 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-14 10:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-31 18:21 [nft PATCH] evaluate: Reject set references in mapping LHS Phil Sutter 2019-11-12 21:45 ` Pablo Neira Ayuso 2019-11-12 22:18 ` Phil Sutter 2019-11-13 10:10 ` Phil Sutter 2019-11-13 23:10 ` Pablo Neira Ayuso 2019-11-14 10:52 ` Phil Sutter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.