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

Nick Alcock
2025-07-22T14:05:16Z

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?