[DTrace-devel] [PATCH v2 04/17] memcpy: bounds-check

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
with...

On 3/14/22 5:30 PM, Nick Alcock wrote:
> This FIXME is pretty easy to implement.
I'd skip this remark.  And...
> Signed-off-by: Nick Alcock <nick.alcock at oracle.com>
> ---
>   libdtrace/dt_cg.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index ea0af2fbbe7a..137033f6413f 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -817,6 +817,8 @@ dt_cg_trace(dt_irlist_t *dlp _dt_unused_, dt_regset_t *drp _dt_unused_,
>   static void
>   dt_cg_memcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src, size_t size)
>   {
> +	uint_t		lbl_ok = dt_irlist_label(dlp);
> +
>   	if (dt_regset_xalloc_args(drp) == -1)
>   		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
>   
> @@ -825,8 +827,13 @@ dt_cg_memcpy(dt_irlist_t *dlp, dt_regset_t *drp, int dst, int src, size_t size)
>   	emit(dlp, BPF_MOV_REG(BPF_REG_3, src));
>   	dt_regset_xalloc(drp, BPF_REG_0);
>   	emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read));
> +
> +	emit(dlp,  BPF_BRANCH_IMM(BPF_JEQ, BPF_REG_0, 0, lbl_ok));
> +	dt_cg_probe_error(yypcb, DT_LBL_NONE, -1, DTRACEFLT_BADADDR, 0);
> +	emitl(dlp, lbl_ok,
> +	      BPF_NOP());
> +
>   	dt_regset_free_args(drp);
> -	/* FIXME: check BPF_REG_0 for error? */
>   	dt_regset_free(drp, BPF_REG_0);
>   }
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.