[DTrace-devel] [PATCH 4/4] Make sure assignment expressions work correctly with tstrings

[Kris Van Hees]

On Mon, Nov 01, 2021 at 05:39:30PM -0400, Eugene Loh wrote:
> On 11/1/21 4:32 PM, Kris Van Hees wrote:
> 
> > On Mon, Nov 01, 2021 at 03:33:01PM -0400, Eugene Loh wrote:
> >>> @@ -2830,6 +2841,35 @@ dt_cg_ternary_op(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> >>>    
> >>>    	emitl(dlp, lbl_post,
> >>>    		   BPF_NOP());
> >>> +
> >>> +	/*
> >>> +	 * Strings complicate things a bit because dn_left and dn_right might
> >>> +	 * actually be temporary strings (tstring) *and* in different slots.
> >>> +	 * We need to allocate a new tstring to hold the result, and copy the
> >>> +	 * value into the new tstring (and free any tstrings in dn_left and
> >>> +	 * dn_right).
> >>> +	 */
> >>> +	if (dt_node_is_string(dnp)) {
> >>> +		/*
> >>> +		 * At this point, dnp->dn_reg holds a pointer to the string we
> >>> +		 * need to copy.  But we want to copy it into a tstring which
> >>> +		 * location is to be stored in dnp->dn_reg.  So, we need to
> >>> +		 * shuffle things a bit.
> >>> +		 */
> >>> +		emit(dlp,  BPF_MOV_REG(BPF_REG_0, dnp->dn_reg));
> >>> +		dt_cg_tstring_alloc(yypcb, dnp);
> >>> +
> >>> +		emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, BPF_REG_FP, DT_STK_DCTX));
> >>> +		emit(dlp,  BPF_LOAD(BPF_DW, dnp->dn_reg, dnp->dn_reg, DCTX_MEM));
> >>> +		emit(dlp,  BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, dnp->dn_tstring->dn_value));
> >>> +
> >>> +		dt_cg_memcpy(dlp, drp, dnp->dn_reg, BPF_REG_0,
> >>> +			     DT_STRLEN_BYTES +
> >>> +			     yypcb->pcb_hdl->dt_options[DTRACEOPT_STRSIZE]);
> >>> +
> >>> +		dt_cg_tstring_free(yypcb, dnp->dn_left);
> >>> +		dt_cg_tstring_free(yypcb, dnp->dn_right);
> >>> +	}
> >>>    }
> >> Should the memcpy not include an extra byte for the NULL terminator?
> > Excellent question!  The answer here is 'no' mainly because we don't do that in
> > other places either but whether that is the perfect behaviour or not is a bit
> > unclear.  The documentation states:
> >
> >    In DTrace, strings are represented as an array of characters terminated by a
> >    null byte (that is, a byte whose value is zero, usually written as '\0').
> >    The visible part of the string is of variable length, depending on the
> >    location of the null byte, but DTrace stores each string in a fixed-size
> >    array so that each probe traces a consistent amount of data. Strings cannot
> >    exceed the length of the predefined string limit. However, the limit can be
> >    modified in your D program or on the dtrace command line by tuning the
> >    strsize option.
> >
> > The terminology shifts a bit near the end of the paragraph when we go from
> > a "fixed-size array" and "consistent amount of data" to "string limit".  The
> > implementation seems to be a bit inconsistent on what the interpretation ought
> > to be.  Whether the setting of strsize is meant to be the limit on the
> > "consistent amount of data" or whether it is that value - 1 to account for
> > the null byte is not clear.
> 
> Okay, but to be clear, regardless of whether we make space for that NULL 
> char internally, we do expect that strsize should be the cap on the 
> number of visible chars, right?  E.g., if Solaris is the reference 
> implementation:

Well, yes, of course.  But since that is not the case right now in other parts
of the code (surely a bug) it makes sense to at least be consistent right now.
This needs a patch that fixes it everywhere.

It is even more fun when you look at the original code base because it is
really unclear whether the current behaviour is intended or a side effect of
the implementation (without actual intent).

> # uname -r
> 5.11
> # /usr/sbin/dtrace -xstrsize=8 -n 'BEGIN { x = "123456789"; 
> printf("|%s|\n", x); exit(0) }'
> dtrace: description 'BEGIN ' matched 1 probe
>   CPU     ID                    FUNCTION:NAME
>    62      1                           :BEGIN |12345678|
> 
> # /usr/sbin/dtrace -xstrsize=7 -n 'BEGIN { x = "123456789"; 
> printf("|%s|\n", x); exit(0) }'
> dtrace: description 'BEGIN ' matched 1 probe
>   CPU     ID                    FUNCTION:NAME
>    62      1                           :BEGIN |1234567|
> 
> # /usr/sbin/dtrace -n 'BEGIN {
>      x = " 123456";
>      printf("%d |%s| |%s|\n", strlen(x), substr(x, 250), x);
>      exit(0);
> }'
> dtrace: description 'BEGIN ' matched 1 probe
>   CPU     ID                    FUNCTION:NAME
>    12      1                           :BEGIN 256 |123456| | 123456|
> 
> That is, 7 chars are visible with strsize=7, 8 with strsize=8, and 256 
> by default.
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel