Re: [PATCH] trace: fix char-array handling

[Kris Van Hees <kris.van.hees@oracle.com>]

On Fri, Feb 07, 2025 at 01:04:53AM -0500, Eugene Loh wrote:
> I don't understand the relevant code very well, but
> Reviewed-by: Eugene Loh <eugene.loh@oracle.com>
> 
> Incidentally, it looks funny to have so many "new" files have 2022
> copyrights on them, but I suppose they're inheriting a lot from pre-existing
> tests... so, okay.

I'll have a look.  Copyright years may need to be updated.

> Also...
> 
> On 2/6/25 11:46, Kris Van Hees wrote:
> > The special handling of strings in the consumer's trace implementation
> > did not correctly account for char-arrays that do not necessarily are
> 
> s/do not necessarily are/are not necessarily/
> 
> > intended to hold string data.  Thr trace action is documented to print
> 
> s/Thr/The/
> 
> > data as an ASCII string if it can be represented as such, i.e. if it
> > comprises only printable characters, optionally terminated by one or
> > more 0-bytes.
> 
> That doesn't read quite right to me.  E.g., 'a' 'b' 0 'c' 0 looks to me like
> "only printable chars, terminated by one or more 0 bytes" yet I think it's
> supposed to be dumped (not printed as a string). I'm not sure and maybe it
> doesn't matter.

'a' 'b' 0 'c' 0 will indeed by bumped as raw bytes because it is not a string
followed by one or more 0-bytes, but rather a potential string that has a
non-printable character (byte) embedded in it, which causes it to not be seen
as a string at all.

> Another possible test is all bytes are 0, but I'm not worried about whether
> such a test is included.
> 
> > The implementation for trace on both producer and consumer side was
> > using the data alignment as a marker to determine whether data should
> > be printed as a string (alignment == 1) or not.  The real alignment
> > of the data should be used, reverting code back to the necessary
> > heuristics in dt_print_bytes().
> > 
> > The dtrace_diftype struct has its dtdt_pad member renamed as dtdt_align.
> > 
> > Added several new test cases to cover various possibilities.
> > 
> > Signed-off-by: Kris Van Hees <kris.van.hees@oracle.com>
> > ---
> >   include/dtrace/dif.h                          |  2 +-
> >   libdtrace/dt_cg.c                             | 16 ++++++----
> >   libdtrace/dt_consume.c                        | 20 ++-----------
> >   libdtrace/dt_parser.c                         |  2 +-
> >   .../actions/trace/tst.array-char-multi-nul.d  | 30 +++++++++++++++++++
> >   .../actions/trace/tst.array-char-multi-nul.r  |  8 +++++
> >   .../trace/tst.array-char-str-multi-nul.d      | 30 +++++++++++++++++++
> >   .../trace/tst.array-char-str-multi-nul.r      |  5 ++++
> >   .../actions/trace/tst.array-char-str-no-nul.d | 30 +++++++++++++++++++
> >   .../actions/trace/tst.array-char-str-no-nul.r |  5 ++++
> >   .../actions/trace/tst.array-char-str.d        | 30 +++++++++++++++++++
> >   .../actions/trace/tst.array-char-str.r        |  5 ++++
> >   .../trace/tst.array-char-unprintable.d        | 30 +++++++++++++++++++
> >   .../trace/tst.array-char-unprintable.r        |  8 +++++
> >   test/unittest/actions/trace/tst.array.d       | 11 +++++--
> >   test/unittest/actions/trace/tst.array.r       |  2 +-
> >   test/unittest/actions/trace/tst.array.r.p     |  6 ----
> >   17 files changed, 206 insertions(+), 34 deletions(-)
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-multi-nul.d
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-multi-nul.r
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-str-multi-nul.d
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-str-multi-nul.r
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-str-no-nul.d
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-str-no-nul.r
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-str.d
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-str.r
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-unprintable.d
> >   create mode 100644 test/unittest/actions/trace/tst.array-char-unprintable.r
> >   delete mode 100755 test/unittest/actions/trace/tst.array.r.p
> > 
> > diff --git a/include/dtrace/dif.h b/include/dtrace/dif.h
> > index 576bda2c..70257b2f 100644
> > --- a/include/dtrace/dif.h
> > +++ b/include/dtrace/dif.h
> > @@ -33,7 +33,7 @@ typedef struct dtrace_diftype {
> >   	uint8_t dtdt_kind;
> >   	uint8_t dtdt_ckind;
> >   	uint8_t dtdt_flags;
> > -	uint8_t dtdt_pad;
> > +	uint8_t dtdt_align;
> >   	uint32_t dtdt_size;
> >   } dtrace_diftype_t;
> > diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> > index 6e74b4b0..a5c9aa09 100644
> > --- a/libdtrace/dt_cg.c
> > +++ b/libdtrace/dt_cg.c
> > @@ -1601,12 +1601,11 @@ static int
> >   dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> >   		dt_pfargv_t *pfp, int arg)
> >   {
> > -	dtrace_diftype_t	vtype;
> >   	dtrace_hdl_t		*dtp = pcb->pcb_hdl;
> >   	dt_irlist_t		*dlp = &pcb->pcb_ir;
> >   	dt_regset_t		*drp = pcb->pcb_regs;
> >   	uint_t			off;
> > -	size_t			size;
> > +	size_t			size, align;
> >   	int			not_null = 0;
> >   	int			cflags = pcb->pcb_stmt->dtsd_clauseflags;
> > @@ -1619,10 +1618,14 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> >   			longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);
> >   		emit(dlp, BPF_MOV_IMM(dnp->dn_reg, dnp->dn_ident->di_id));
> >   		size = sizeof(dnp->dn_ident->di_id);
> > +		align = size;
> >   	} else {
> > +		dtrace_diftype_t	vtype;
> > +
> >   		dt_cg_node(dnp, dlp, drp);
> >   		dt_node_diftype(dtp, dnp, &vtype);
> >   		size = vtype.dtdt_size;
> > +		align = vtype.dtdt_align;
> >   		/*
> >   		 * A DEREF of a REF node does not get resolved in dt_cg_node()
> > @@ -1664,7 +1667,7 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> >   	if (dt_node_is_scalar(dnp) || dt_node_is_float(dnp) ||
> >   	    dnp->dn_kind == DT_NODE_AGG) {
> > -		off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size, size, pfp,
> > +		off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size, align, pfp,
> >   				 arg);
> >   		emit(dlp, BPF_STOREX(size, BPF_REG_9, off, dnp->dn_reg));
> > @@ -1678,8 +1681,8 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> >   			dt_cg_check_ptr_arg(dlp, drp, dnp, NULL);
> >   		TRACE_REGSET("store_val(): Begin ");
> > -		off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size + 1,
> > -				 1, pfp, arg);
> > +		off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size + 1, 1, pfp,
> > +				 arg);
> >   		/*
> >   		 * Copy the string data (no more than STRSIZE + 1 bytes) to the
> > @@ -1706,7 +1709,8 @@ dt_cg_store_val(dt_pcb_t *pcb, dt_node_t *dnp, dtrace_actkind_t kind,
> >   	/* Handle tracing of by-ref values (arrays, struct, union). */
> >   	if ((dnp->dn_flags & DT_NF_REF) || (arg & DT_NF_REF)) {
> > -		off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size, 2, pfp, arg);
> > +		off = dt_rec_add(dtp, dt_cg_fill_gap, kind, size, align, pfp,
> > +				 arg);
> >   		TRACE_REGSET("store_val(): Begin ");
> >   		if (!not_null)
> > diff --git a/libdtrace/dt_consume.c b/libdtrace/dt_consume.c
> > index 74004c69..58a2ead9 100644
> > --- a/libdtrace/dt_consume.c
> > +++ b/libdtrace/dt_consume.c
> > @@ -1921,23 +1921,9 @@ dt_print_trace(dtrace_hdl_t *dtp, FILE *fp, dtrace_recdesc_t *rec,
> >   	if (dtp->dt_options[DTRACEOPT_RAWBYTES] != DTRACEOPT_UNSET)
> >   		return dt_print_rawbytes(dtp, fp, data, rec->dtrd_size);
> > -	/*
> > -	 * String data can be recognized as a non-scalar data item with
> > -	 * alignment == 1.
> > -	 * Any other non-scalar data items are printed as a byte stream.
> > -	 */
> > -	if (rec->dtrd_arg == DT_NF_REF) {
> > -		char	*s = (char *)data;
> > -
> > -		if (rec->dtrd_alignment > 1)
> > -			return dt_print_rawbytes(dtp, fp, data, rec->dtrd_size);
> > -
> > -		/* We have a string.  Print it. */
> > -		if (quiet)
> > -			return dt_printf(dtp, fp, "%s", s);
> > -		else
> > -			return dt_printf(dtp, fp, "  %-33s", s);
> > -	}
> > +	/* Handle non-scalar data. */
> > +	if (rec->dtrd_arg == DT_NF_REF)
> > +		return dt_print_bytes(dtp, fp, data, rec->dtrd_size, 33, quiet);
> >   	/*
> >   	 * Differentiate between signed and unsigned numeric values.
> > diff --git a/libdtrace/dt_parser.c b/libdtrace/dt_parser.c
> > index 325ba881..eefe8341 100644
> > --- a/libdtrace/dt_parser.c
> > +++ b/libdtrace/dt_parser.c
> > @@ -4914,7 +4914,7 @@ dt_node_diftype(dtrace_hdl_t *dtp, const dt_node_t *dnp, dtrace_diftype_t *tp)
> >   	}
> >   	tp->dtdt_flags = (dnp->dn_flags & DT_NF_REF) ? DIF_TF_BYREF : 0;
> > -	tp->dtdt_pad = 0;
> > +	tp->dtdt_align = ctf_type_align(dnp->dn_ctfp, dnp->dn_type);
> >   	tp->dtdt_size = ctf_type_size(dnp->dn_ctfp, dnp->dn_type);
> >   }
> > diff --git a/test/unittest/actions/trace/tst.array-char-multi-nul.d b/test/unittest/actions/trace/tst.array-char-multi-nul.d
> > new file mode 100644
> > index 00000000..187fb711
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-multi-nul.d
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2022, 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: The trace() action prints a char-array of printable characters
> > + *	      with multiple 0-bytes in its content.
> > + *
> > + * SECTION: Actions and Subroutines/trace()
> > + */
> > +
> > +char n[9];
> > +
> > +BEGIN
> > +{
> > +	n[0] = 'a';
> > +	n[1] = 'A';
> > +	n[2] = 0;
> > +	n[3] = 'B';
> > +	n[4] = 0;
> > +	n[5] = 'C';
> > +	n[6] = 0;
> > +	n[7] = 'D';
> > +	n[8] = 'e';
> > +	trace(n);
> > +	exit(0);
> > +}
> > diff --git a/test/unittest/actions/trace/tst.array-char-multi-nul.r b/test/unittest/actions/trace/tst.array-char-multi-nul.r
> > new file mode 100644
> > index 00000000..c1361e75
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-multi-nul.r
> > @@ -0,0 +1,8 @@
> > +                   FUNCTION:NAME
> > +                          :BEGIN
> > +             0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f  0123456789abcdef
> > +         0: 61 41 00 42 00 43 00 44 65                       aA.B.C.De
> > +
> > +
> > +-- @@stderr --
> > +dtrace: script 'test/unittest/actions/trace/tst.array-char-multi-nul.d' matched 1 probe
> > diff --git a/test/unittest/actions/trace/tst.array-char-str-multi-nul.d b/test/unittest/actions/trace/tst.array-char-str-multi-nul.d
> > new file mode 100644
> > index 00000000..75164cd3
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-str-multi-nul.d
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2022, 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: The trace() action prints a char-array of printable characters
> > + *	      followed y by multiple 0-bytes correctly.
> > + *
> > + * SECTION: Actions and Subroutines/trace()
> > + */
> > +
> > +char n[9];
> > +
> > +BEGIN
> > +{
> > +	n[0] = 'a';
> > +	n[1] = 'A';
> > +	n[2] = 'b';
> > +	n[3] = 'B';
> > +	n[4] = 'c';
> > +	n[5] = 0;
> > +	n[6] = 0;
> > +	n[7] = 0;
> > +	n[8] = 0;
> > +	trace(n);
> > +	exit(0);
> > +}
> > diff --git a/test/unittest/actions/trace/tst.array-char-str-multi-nul.r b/test/unittest/actions/trace/tst.array-char-str-multi-nul.r
> > new file mode 100644
> > index 00000000..1951ae69
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-str-multi-nul.r
> > @@ -0,0 +1,5 @@
> > +                   FUNCTION:NAME
> > +                          :BEGIN   aAbBc
> > +
> > +-- @@stderr --
> > +dtrace: script 'test/unittest/actions/trace/tst.array-char-str-multi-nul.d' matched 1 probe
> > diff --git a/test/unittest/actions/trace/tst.array-char-str-no-nul.d b/test/unittest/actions/trace/tst.array-char-str-no-nul.d
> > new file mode 100644
> > index 00000000..532c3519
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-str-no-nul.d
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2022, 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: The trace() action prints a char-aarray of printable characters
> > + *	      (not terminated) correctly.
> > + *
> > + * SECTION: Actions and Subroutines/trace()
> > + */
> > +
> > +char n[9];
> > +
> > +BEGIN
> > +{
> > +	n[0] = 'a';
> > +	n[1] = 'A';
> > +	n[2] = 'b';
> > +	n[3] = 'B';
> > +	n[4] = 'c';
> > +	n[5] = 'C';
> > +	n[6] = 'd';
> > +	n[7] = 'D';
> > +	n[8] = 'e';
> > +	trace(n);
> > +	exit(0);
> > +}
> > diff --git a/test/unittest/actions/trace/tst.array-char-str-no-nul.r b/test/unittest/actions/trace/tst.array-char-str-no-nul.r
> > new file mode 100644
> > index 00000000..3d56e520
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-str-no-nul.r
> > @@ -0,0 +1,5 @@
> > +                   FUNCTION:NAME
> > +                          :BEGIN   aAbBcCdDe
> > +
> > +-- @@stderr --
> > +dtrace: script 'test/unittest/actions/trace/tst.array-char-str-no-nul.d' matched 1 probe
> > diff --git a/test/unittest/actions/trace/tst.array-char-str.d b/test/unittest/actions/trace/tst.array-char-str.d
> > new file mode 100644
> > index 00000000..70f6371f
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-str.d
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2022, 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: The trace() action prints a char-array of printable characters
> > + *	      (terminated) correctly.
> > + *
> > + * SECTION: Actions and Subroutines/trace()
> > + */
> > +
> > +char n[9];
> > +
> > +BEGIN
> > +{
> > +	n[0] = 'a';
> > +	n[1] = 'A';
> > +	n[2] = 'b';
> > +	n[3] = 'B';
> > +	n[4] = 'c';
> > +	n[5] = 'C';
> > +	n[6] = 'd';
> > +	n[7] = 'D';
> > +	n[8] = '\0';
> > +	trace(n);
> > +	exit(0);
> > +}
> > diff --git a/test/unittest/actions/trace/tst.array-char-str.r b/test/unittest/actions/trace/tst.array-char-str.r
> > new file mode 100644
> > index 00000000..9778ddd2
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-str.r
> > @@ -0,0 +1,5 @@
> > +                   FUNCTION:NAME
> > +                          :BEGIN   aAbBcCdD
> > +
> > +-- @@stderr --
> > +dtrace: script 'test/unittest/actions/trace/tst.array-char-str.d' matched 1 probe
> > diff --git a/test/unittest/actions/trace/tst.array-char-unprintable.d b/test/unittest/actions/trace/tst.array-char-unprintable.d
> > new file mode 100644
> > index 00000000..d88df369
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-unprintable.d
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Oracle Linux DTrace.
> > + * Copyright (c) 2022, 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: The trace() action prints a char-array with a non-printable
> > + *	      character correctly.
> > + *
> > + * SECTION: Actions and Subroutines/trace()
> > + */
> > +
> > +char n[9];
> > +
> > +BEGIN
> > +{
> > +	n[0] = 'a';
> > +	n[1] = 'A';
> > +	n[2] = 'b';
> > +	n[3] = 'B';
> > +	n[4] = 'c';
> > +	n[5] = 5;
> > +	n[6] = 'd';
> > +	n[7] = 'D';
> > +	n[8] = '\0';
> > +	trace(n);
> > +	exit(0);
> > +}
> > diff --git a/test/unittest/actions/trace/tst.array-char-unprintable.r b/test/unittest/actions/trace/tst.array-char-unprintable.r
> > new file mode 100644
> > index 00000000..1f84fcfa
> > --- /dev/null
> > +++ b/test/unittest/actions/trace/tst.array-char-unprintable.r
> > @@ -0,0 +1,8 @@
> > +                   FUNCTION:NAME
> > +                          :BEGIN
> > +             0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f  0123456789abcdef
> > +         0: 61 41 62 42 63 05 64 44 00                       aAbBc.dD.
> > +
> > +
> > +-- @@stderr --
> > +dtrace: script 'test/unittest/actions/trace/tst.array-char-unprintable.d' matched 1 probe
> > diff --git a/test/unittest/actions/trace/tst.array.d b/test/unittest/actions/trace/tst.array.d
> > index 104d42e1..0779d90f 100644
> > --- a/test/unittest/actions/trace/tst.array.d
> > +++ b/test/unittest/actions/trace/tst.array.d
> > @@ -1,6 +1,6 @@
> >   /*
> >    * Oracle Linux DTrace.
> > - * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
> > + * Copyright (c) 2022, 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.
> >    */
> > @@ -11,8 +11,15 @@
> >    * SECTION: Actions and Subroutines/trace()
> >    */
> > +short n[5];
> > +
> >   BEGIN
> >   {
> > -	trace(curthread->comm);
> > +	n[0] = 0x1234;
> > +	n[1] = 0x5678;
> > +	n[2] = 0x0000;
> > +	n[3] = 0x8765;
> > +	n[4] = 0x4321;
> > +	trace(n);
> >   	exit(0);
> >   }
> > diff --git a/test/unittest/actions/trace/tst.array.r b/test/unittest/actions/trace/tst.array.r
> > index fbb674b6..52ff28ec 100644
> > --- a/test/unittest/actions/trace/tst.array.r
> > +++ b/test/unittest/actions/trace/tst.array.r
> > @@ -1,7 +1,7 @@
> >                      FUNCTION:NAME
> >                             :BEGIN
> >                0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f  0123456789abcdef
> > -         0: 64 74 72 61 63 65 00 00 00 00 00 00 00 00 00 00  dtrace..........
> > +         0: 34 12 78 56 00 00 65 87 21 43                    4.xV..e.!C
> >   -- @@stderr --
> > diff --git a/test/unittest/actions/trace/tst.array.r.p b/test/unittest/actions/trace/tst.array.r.p
> > deleted file mode 100755
> > index b8cc8daf..00000000
> > --- a/test/unittest/actions/trace/tst.array.r.p
> > +++ /dev/null
> > @@ -1,6 +0,0 @@
> > -#!/usr/bin/gawk -f
> > -
> > -# Some Linux kernel versions leave garbage at the end of the string.
> > -{ sub(/( [0-9A-F]{2}){9}  /, " 00 00 00 00 00 00 00 00 00  "); }
> > -{ sub(/  dtrace\..{9}/, "  dtrace.........."); }
> > -{ print; }