El 16 de mayo de 2019 16:39:42 CEST, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> escribió:
>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.
>
Ok. I will send a new patch series. Thanks.
>> + return -1;
>> + }
>> }
>> break;
>> case EXPR_MAP:
>>
>> That works for all the cases that you mentioned above. What do you
>think?
>>
>> Thanks :-)
>>
>> > Cheers, Phil
>> >
