Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > diff --git a/src/evaluate.c b/src/evaluate.c
> > index d099be137cb3..0c8af09492d1 100644
> > --- a/src/evaluate.c
> > +++ b/src/evaluate.c
> [...]
> > @@ -1704,10 +1706,48 @@ static int expr_evaluate_concat(struct eval_ctx *ctx, struct expr **expr)
> > if (list_member_evaluate(ctx, &i) < 0)
> > return -1;
> >
> > - if (i->etype == EXPR_SET)
> > + switch (i->etype) {
> > + case EXPR_VALUE:
> > + case EXPR_UNARY:
> > + case EXPR_BINOP:
> > + case EXPR_RELATIONAL:
> > + case EXPR_CONCAT:
> > + case EXPR_MAP:
> > + case EXPR_PAYLOAD:
> > + case EXPR_EXTHDR:
> > + case EXPR_META:
> > + case EXPR_RT:
> > + case EXPR_CT:
> > + case EXPR_SET_ELEM:
> > + case EXPR_NUMGEN:
> > + case EXPR_HASH:
> > + case EXPR_FIB:
> > + case EXPR_SOCKET:
> > + case EXPR_OSF:
> > + case EXPR_XFRM:
>
> I am expecting more new selector expressions here that would need to
> be added and I think it is less likely to see new constant expressions
> in the future, so maybe reverse this logic ...
>
> if (i->etype == EXPR_RANGE ||
> i->etype == EXPR_PREFIX) {
> /* allowed on RHS (e.g. th dport . mark { 1-65535 . 42 }
> * ~~~~~~~~ allowed
> * but not on LHS (e.g 1-4 . mark { ...}
> * ~~~ illegal
> ...
>
> ... and let anything else be accepted?
I prefer "accept whats safe and reject rest" but I can invert
if you want.
> > + * EXPR_SET_ELEM (is used as RHS).
> > + */
> > + if (ctx->recursion.list > 0)
> > + break;
>
> So recursion.list is used to provide context to identify this is rhs,
> correct?
Yes.
> Is your intention is to use this recursion.list to control to
> deeper recursions in a follow up patch?
No, what did you have in mind?
I could see adding new members to ctx->recursion to control other
possible recursions in addition to what we have now.
But I don't see other uses for .list at this time.
> Not related, but if goal is to provide context then I also need more
> explicit context hints for bitfield payload and bitwise expressions
> where the evaluation needs to be different depending on where the
> expression is located (not the same if the expression is either used
> as selector or as lhs/rhs of assignment).
>
> I don't know yet how such new context enum to modify evaluation
> behaviour will look, so we can just use recursion.list by now, I don't
> want to block this fix.
OK. Yes, it would also work if there was some different "where am I"
indicator, e.g. if (ctx->expr_side == CTX_EXPR_LHS) or whatever.
This fix isn't urgent, we can keep it back and come back to this
if you prefer to first work on the ctx hint extensions.
