Re: [PATCH 4/4] Pad strings in the output buffer with NUL bytes after terminating byte

[Nick Alcock <nick.alcock@oracle.com>]

On 25 Mar 2025, eugene loh uttered the following:

> From: Eugene Loh <eugene.loh@oracle.com>
>
> The consumer checks if there are non-NUL bytes after the terminating
> NUL byte to decide whether to print the contents of the output buffer
> as a string or as raw bytes.  So, for strings, make sure that the
> string is padded with NUL bytes.

Ugh. I guess there's no choice if we want trace() to work properly.

A use for strncpy()! except that this is BPF :)

> Signed-off-by: Eugene Loh <eugene.loh@oracle.com>

Reviewed-by: Nick Alcock <nick.alcock@oracle.com>

because basing decisions on uninitialized data is bad.

(Modulo nit below.)

> ---
>  libdtrace/dt_cg.c                      | 46 ++++++++++++++++++--------
>  test/unittest/error/tst.trace_string.d | 26 +++++++++++++++
>  2 files changed, 59 insertions(+), 13 deletions(-)
>  create mode 100644 test/unittest/error/tst.trace_string.d
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 9b3592b9c..6dcf4cd3d 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -1596,6 +1596,22 @@ dt_cg_check_ptr_arg(dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dnp,
>  
>  static void dt_cg_setx(dt_irlist_t *dlp, int reg, uint64_t x);
>  
> +/*
> + * Store a pointer to the 'memory block of zeros' in reg.
> + */
> +static void
> +dt_cg_zerosptr(int reg, dt_irlist_t *dlp, dt_regset_t *drp)
> +{
> +	dtrace_hdl_t	*dtp = yypcb->pcb_hdl;
> +	dt_ident_t	*zero_off = dt_dlib_get_var(dtp, "ZERO_OFF");
> +
> +	dt_cg_access_dctx(reg, dlp, drp, DCTX_STRTAB);
> +	emite(dlp, BPF_ALU64_IMM(BPF_ADD, reg, -1), zero_off);
> +}
> +
> +/*
> + * Store a value to the output buffer.
> + */
>  static int
>  dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>  		dt_pfargv_t *pfp, int arg)
> @@ -1676,6 +1692,7 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>  
>  		goto ok;
>  	} else if (dt_node_is_string(dnp)) {
> +		uint_t	lbl_ok = dt_irlist_label(dlp);
>  		size_t	strsize = dtp->dt_options[DTRACEOPT_STRSIZE];
>  
>  		if (!not_null)
> @@ -1702,6 +1719,22 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
>  		dt_regset_xalloc(drp, BPF_REG_0);
>  		emit(dlp, BPF_CALL_HELPER(BPF_FUNC_probe_read_str));
>  		dt_regset_free_args(drp);
> +
> +		/*
> +		 * Pad the rest with zeroes, if necessary.
> +		 */
> +		emit(dlp,  BPF_BRANCH_IMM(BPF_JGE, BPF_REG_0, strsize + 1, lbl_ok));
> +		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));
> +		emit(dlp,  BPF_ALU64_REG(BPF_ADD, BPF_REG_1, BPF_REG_0));
> +		emit(dlp,  BPF_MOV_IMM(BPF_REG_2, strsize + 1));
> +		emit(dlp,  BPF_ALU64_REG(BPF_SUB, BPF_REG_2, BPF_REG_0));
> +		dt_cg_zerosptr(BPF_REG_3, dlp, drp);
> +		emit(dlp,  BPF_CALL_HELPER(dtp->dt_bpfhelper[BPF_FUNC_probe_read_kernel]));

Much faster than using a loop in all but the smallest cases, I suppose.
(And the zero block is presumably always big enough... yes, it is.)

> diff --git a/test/unittest/error/tst.trace_string.d b/test/unittest/error/tst.trace_string.d
> new file mode 100644
> index 000000000..4b06aef88
> --- /dev/null
> +++ b/test/unittest/error/tst.trace_string.d
> @@ -0,0 +1,26 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: Test ERROR probe firing.

... is that really what you want to describe this test as?

> + * SECTION: dtrace Provider
> + */
> +
> +#pragma D option quiet
> +
> +ERROR
> +{
> +	trace("Error fired");

.... given that its stated purpose is quite different, why not describe
that purpose?