[DTrace-devel] dt_regset_free and spills (was Re: [PATCH v2 04/17] memcpy: bounds-check)

[Nick Alcock]

On 15 Mar 2022, Eugene Loh via DTrace-devel stated:

> Not a big deal, but I'd place the fix more like where the FIXME comment was.  I realize we're not 100% consistent about this, but
> check out dg_cg_act_[u]stack().  The order is:
>
>     emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
>     dt_regset_free_args(drp);
>     emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_ok));
>     dt_regset_free(drp, BPF_REG_0);
>     dt_cg_probe_error(yypcb, DT_LBL_NONE, -1, DTRACEFLT_BADADDR, 0);
>     emitl(dlp, lbl_ok,
>           BPF_NOP());
>
> That is, we free_args() as soon as possible and then free(%r0) as soon as possible.  Just a matter of preference, though.

That's buggy: the dt_regset_free can generate fills, which need to be
invoked even (especially) if we go down the lbl_ok path. Since we cannot
free before the conditional, we must free after it: we then optimize by
dropping the unnecessary free right before dt_cg_probe_error(), which
leaves us with one free, after lbl_ok.

The dt_regset_free_args can and should move up, though (adjusted).

(Yes, this bit me, although not in this particular function. I keep
meaning to audit all the frees in the codebase for similar bugs.)

... hm, OK, I just audited. There are a bunch (mostly fixed in my tree),
but most are at the ends of actions where I suspect we don't care if
spills are lost. But... there's one in dt_cg_store_var's
associative-array part which is troublesome:

		if (dt_regset_xalloc_args(drp) == -1)
			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
/* ... */
		dt_regset_free(drp, dnp->dn_left->dn_args->dn_reg);
/* ... */
		dt_regset_xalloc(drp, BPF_REG_0);
		emite(dlp, BPF_CALL_FUNC(idp->di_id), idp);
		dt_regset_free_args(drp);
		lbl_done = dt_irlist_label(dlp);
		emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, dnp->dn_reg, 0, lbl_done));

		if ((reg = dt_regset_alloc(drp)) == -1)
			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);

		emit(dlp,  BPF_MOV_REG(reg, BPF_REG_0));
		dt_regset_free(drp, BPF_REG_0);
/* (that's one free, on one branch) */
/* ... */
		emitl(dlp, lbl_done,
			   BPF_NOP());

/* ... and now a leak.  But we can't put a dt_regset_free on this branch
   because the code generator would now think we're freeing it twice!
   What's worse, in the dnp->dn_reg != 0 case, we *have* already freed
   it and done any fills needed. The latter is easily fixed with a
   jumped-over hunk specifically for the dnp->dn_reg == 0 case, but
   we still have the problem that dt_regset thinks the register is
   freed when in fact it was only freed on one branch.  */

I think in this case we have no choice but to *not* free on the true
branch and consume the reg until after lbl_done :( adjusted accordingly,
but it makes me feel icky.

(Done. Auditing and fixing the rest :) )

-- 
NULL && (void)