[DTrace-devel] [PATCH 03/12] Fix alloca() non-constant size check

[Eugene Loh]

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

Good enough, I guess, albeit with the usual number of questions.

First, I am not real familiar with the alloca()/scratch stuff.  So maybe 
Nick wants to take a look.

Specifically, I do not understand the whole "8" thing.  (I guess there 
is an initial 8-byte padding to represent a NULL pointer. This padding 
is so invasive, hard-coded into every access to the scratch area.  Might 
it have been better placed at the *end* of the scratch area?  I don't 
know.  But, again, this eccentricity is being exposed in so many places 
in the code... that magic "8" value is all over the place.)

I don't get why DTRACEOPT_SCRATCHSIZE is not the value of the 
scratchsize option, as the name would suggest (at least to me).

Having that "8" included in DTRACEOPT_SCRATCHSIZE also makes me wonder
if these checks are right:

     dt_bpf.c:
         if (dtp->dt_options[DTRACEOPT_SCRATCHSIZE] > 0)
             create_gmap(dtp, "scratchmem", ...);

     dt_cg.c:
         if (dtp->dt_options[DTRACEOPT_SCRATCHSIZE] > 0)
                 DT_CG_STORE_MAP_PTR("scratchmem", DCTX_SCRATCHMEM);

As for this specific patch:


diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
@@ -4177,8 +4177,8 @@ dt_cg_subr_alloca(dt_node_t *dnp, dt_irlist_t 
*dlp, dt_regset_t *drp)
      emit(dlp,  BPF_ALU64_IMM(BPF_ADD, next, 7));
      emit(dlp,  BPF_ALU64_IMM(BPF_AND, next, (int) -8));

-    emit(dlp,  BPF_BRANCH_IMM(BPF_JGT, next, opt_scratchsize + 8, 
lbl_err));
+    emit(dlp,  BPF_BRANCH_IMM(BPF_JGT, next, opt_scratchsize, lbl_err));
-    emit(dlp,  BPF_BRANCH_IMM(BPF_JLE, dnp->dn_reg, opt_scratchsize + 8,
+    emit(dlp,  BPF_BRANCH_IMM(BPF_JLE, dnp->dn_reg, opt_scratchsize - 8,
                    lbl_ok));
      emitl(dlp, lbl_err,
             BPF_NOP());

I can guess why we check dnp->dn_reg, but wouldn't mind an explanation.  
E.g., I could also believe that if we have checked that next is 
in-bounds, then certainly dnp->dn_reg must also be, making that check 
unnecessary.  (I could argue to myself that there is an edge case, but 
it's probably better for me to ask for explanation.)

@@ -4199,7 +4199,7 @@ dt_cg_subr_bcopy(dt_node_t *dnp, dt_irlist_t *dlp, 
dt_regset_t *drp)
      dt_node_t    *src = dnp->dn_args;
      dt_node_t    *dst = src->dn_list;
      dt_node_t    *size = dst->dn_list;
-    int        maxsize = 
yypcb->pcb_hdl->dt_options[DTRACEOPT_SCRATCHSIZE];
+    int        maxsize = 
yypcb->pcb_hdl->dt_options[DTRACEOPT_SCRATCHSIZE] - 8;
      uint_t        lbl_badsize = dt_irlist_label(dlp);
      uint_t        lbl_ok = dt_irlist_label(dlp);

Why is there a maxsize at all?  Put another way, why is it used to check 
size?  Why doesn't the dt_cg_alloca_access_check() that follows the 
check suffice?