[DTrace-devel] [PATCH 2/4] Store strings in the trace output buffer without length prefixes

[Kris Van Hees]

On Fri, Jan 28, 2022 at 02:27:48PM -0500, Eugene Loh via DTrace-devel wrote:
> Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
> with one question:
> 
> On 1/26/22 11:28 AM, Kris Van Hees via DTrace-devel wrote:
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > @@ -932,34 +932,29 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> >   	} else if (dt_node_is_string(dnp)) {
> > +		size_t	strsize = pcb->pcb_hdl->dt_options[DTRACEOPT_STRSIZE];
> > +
> 
> Isn't strsize==size?  That is, no need to define strsize... just use size
> instead?

It should be but I am not 100% certain we guarantee that in all cases.  This is
a safety net in case there is a possibility of a string being passed that is
larger than strsize.  I believe it might be possible to encounter that situation
if we are trying to print a kernel string (declared at the kernel level as a
char array and we have its type through CTF), and we 'cast' that to string.

There is no downside really from having this safety net, so I think I would
prefer to keep it unless I can prove it is unnecessary.

> >   		dt_cg_check_notnull(dlp, drp, dnp->dn_reg);
> >   		TRACE_REGSET("store_val(): Begin ");
> > -		off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, kind,
> > -				 size + DT_STRLEN_BYTES + 1, 1, pfp, arg);
> > -
> > -		/*
> > -		 * Copy the length of the string from the source string as a
> > -		 * half-word (2 bytes) into the buffer at [%r9 + off].
> > -		 */
> > -		emit(dlp, BPF_LOAD(BPF_H, BPF_REG_0, dnp->dn_reg, 0));
> > -		emit(dlp, BPF_STORE(BPF_H, BPF_REG_9, off, BPF_REG_0));
> > +		off = dt_rec_add(pcb->pcb_hdl, dt_cg_fill_gap, kind, size + 1,
> > +				 1, pfp, arg);
> >   		/*
> >   		 * Copy the string data (no more than STRSIZE + 1 bytes) to the
> > -		 * buffer at [%r9 + off + DT_STRLEN_BYTES].  We (ab)use the
> > -		 * fact that probe_read_str) stops at the terminating NUL byte.
> > +		 * buffer at (%r9 + off).  We depend on the fact that
> > +		 * probe_read_str() stops at the terminating NUL byte.
> >   		 */
> >   		if (dt_regset_xalloc_args(drp) == -1)
> >   			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> >   		emit(dlp, BPF_MOV_REG(BPF_REG_1, BPF_REG_9));
> > -		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off + DT_STRLEN_BYTES));
> > -		emit(dlp, BPF_MOV_IMM(BPF_REG_2, size + 1));
> > +		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_1, off));
> > +		emit(dlp, BPF_MOV_IMM(BPF_REG_2, strsize + 1));
> >   		emit(dlp, BPF_MOV_REG(BPF_REG_3, dnp->dn_reg));
> > +		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, DT_STRLEN_BYTES));
> >   		dt_regset_free(drp, dnp->dn_reg);
> >   		dt_cg_tstring_free(pcb, dnp);
> > -		emit(dlp, BPF_ALU64_IMM(BPF_ADD, BPF_REG_3, DT_STRLEN_BYTES));
> >   		dt_regset_xalloc(drp, BPF_REG_0);
> >   		emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read_str));
> >   		dt_regset_free_args(drp);
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel