[DTrace-devel] [PATCH] trace: fix char-array handling
Kris Van Hees
kris.van.hees at oracle.com
Fri Feb 7 06:08:42 UTC 2025
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 at 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 at 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; }
More information about the DTrace-devel
mailing list