On Thu, May 16, 2019 at 01:58:17PM +0200, Fernando Fernandez Mancera wrote:
> Hi!
>
> On 5/15/19 10:31 PM, Phil Sutter wrote:
> > Hi,
> >
> > On Wed, May 15, 2019 at 09:56:11PM +0200, Fernando Fernandez Mancera wrote:
> >> Hi Phil,
> >>
> >> On 5/15/19 9:26 PM, Phil Sutter wrote:
> >>> Hi Pablo,
> >>>
> >>> On Wed, May 15, 2019 at 05:21:32PM +0200, Pablo Neira Ayuso wrote:
> >>>> On Wed, May 15, 2019 at 01:46:17PM +0200, Phil Sutter wrote>> [...]
> >>>> '@<something>' is currently allowed, as any arbitrary string can be
> >>>> placed in between strings - although in some way this is taking us
> >>>> back to the quote debate that needs to be addressed. If we want to
> >>>> disallow something enclosed in quotes then we'll have to apply this
> >>>> function everywhere we allow variables.
> >>>
> >>> Oh, sorry. I put those ticks in there just to quote the value, not as
> >>> part of the value. The intention was to point out that something like:
> >>>
> >>> | define foo = @set1
> >>> | add rule ip t c jump $foo
> >>>
> >>> Might pass evaluation stage and since there is a special case for things
> >>> starting with '@' in symbol_expr, the added rule would turn into
> >>>
> >>> | add rule ip t c jump set1
> >>>
> >>> We could detect this situation by checking expr->symtype.
> >>>
> >>
> >> I agree about that. We could check if the symbol type is SYMBOL_VALUE.
> >> But I am not sure about where should we do it, maybe in the parser?
> >>
> >>> On the other hand, can we maybe check if given string points to an
> >>> *existing* chain in verdict_type_parse()? Or will that happen later
> >>> anyway?
> >>>
> >>
> >> It happens later, right now if the given string does not point to an
> >> existing chain it returns the usual error for this situation. e.g
> >
> > I just played around a bit and could provoke some segfaults:
> >
> > * define foo = @set1 (a set named 'set1' must exist)
> > * define foo = { 1024 }
> > * define foo = *
> >
> > I didn't check how we could avoid those. Maybe this is even follow-up
> > work, but we should definitely try to address those eventually.
> >
>
> I have been working on fixing this. I propose the following fix.
>
> diff --git a/src/evaluate.c b/src/evaluate.c
> index 8394037..edab370 100644
> --- a/src/evaluate.c
> +++ b/src/evaluate.c
> @@ -1950,6 +1950,12 @@ static int stmt_evaluate_verdict(struct eval_ctx
> *ctx, struct stmt *stmt)
> if (stmt->expr->chain != NULL) {
> if (expr_evaluate(ctx, &stmt->expr->chain) < 0)
> return -1;
> + if (stmt->expr->chain->etype != EXPR_SYMBOL ||
> + stmt->expr->chain->symtype != SYMBOL_VALUE) {
> + BUG("invalid verdict chain expression %s\n",
> + expr_name(stmt->expr->chain));
Instead of BUG(), I'd suggest you do proper error reporting.
> + return -1;
> + }
> }
> break;
> case EXPR_MAP:
>
> That works for all the cases that you mentioned above. What do you think?
>
> Thanks :-)
>
> > Cheers, Phil
> >
