On Tue, Feb 09, 2021 at 04:54:33PM +0100, Pablo Neira Ayuso wrote: > On Tue, Feb 09, 2021 at 04:53:19PM +0100, Pablo Neira Ayuso wrote: > > On Tue, Feb 09, 2021 at 04:50:30PM +0100, Pablo Neira Ayuso wrote: > > > On Tue, Feb 09, 2021 at 03:11:51PM +0100, Phil Sutter wrote: > > > > Hi Pablo, > > > > > > > > On Tue, Feb 09, 2021 at 02:15:11PM +0100, Pablo Neira Ayuso wrote: > > > > > On Wed, Feb 03, 2021 at 11:45:07AM +0100, Phil Sutter wrote: > > > > > > On Wed, Feb 03, 2021 at 01:38:32AM +0100, Pablo Neira Ayuso wrote: > > > > > > > On Tue, Jan 26, 2021 at 06:55:02PM +0100, Phil Sutter wrote: > > > > > > > > erec_print() unconditionally dereferences erec->locations->indesc, so > > > > > > > > make sure it is valid when either creating an erec or adding a location. > > > > > > > > > > > > > > I guess your're trigger a bug where erec is indesc is NULL, thing is > > > > > > > that indesc should be always set on. Is there a reproducer for this bug? > > > > > > > > > > > > Yes, exactly. I hit it when trying to clean up the netdev family reject > > > > > > support, while just "hacking around". You can trigger it with the > > > > > > following change: > > > > > > > > > > > > | --- a/src/evaluate.c > > > > > > | +++ b/src/evaluate.c > > > > > > | @@ -2718,7 +2718,7 @@ static int stmt_evaluate_reject_bridge(struct eval_ctx *ctx, struct stmt *stmt, > > > > > > | const struct proto_desc *desc; > > > > > > | > > > > > > | desc = ctx->pctx.protocol[PROTO_BASE_LL_HDR].desc; > > > > > > | - if (desc != &proto_eth && desc != &proto_vlan && desc != &proto_netdev) > > > > > > | + if (desc != &proto_eth && desc != &proto_vlan) > > > > > > | return stmt_binary_error(ctx, > > > > > > | &ctx->pctx.protocol[PROTO_BASE_LL_HDR], > > > > > > | stmt, "unsupported link layer protocol"); > > > > > > > > > > I'm attaching fix. > > > > > > > > > > Looks like call to stmt_binary_error() parameters are not in the right > > > > > order, &ctx->pctx.protocol[PROTO_BASE_LL_HDR] has indesc. > > > > > > > > Thanks for addressing the root problem! > > > > > > > > > Probably add a bugtrap to erec to check that indesc is always set on > > > > > accordingly instead? > > > > > > > > Is it better than just sanitizing input to error functions? After all we > > > > just want to make sure users see the error message, right? Catching > > > > the programming mistake (wrong args passed to __stmt_binary_error()) > > > > IMHO is useful only if we can compile-time assert it. Otherwise we risk > > > > hiding error info from user. > > > > > > I see. I don't see a way to catch this at compile time. > > > > > > Push out your patch and I'll push mine too for correctness. DONE. > > Hm, one second: Probably set internal_indesc for autogenerated > > dependencies? > > Either way, it's just changing where internal_indesc is set. > > Probably not worth spending more cycles on this issue. ACK. :) Cheers, Phil
