[DTrace-devel] [PATCH v2] cg: fix store/load for struct and union members for alloca pointers
Eugene Loh
eugene.loh at oracle.com
Thu Aug 31 20:30:38 UTC 2023
Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
There should be .r files for the tests.
There is no point in separate load and store tests. Specifically, the
load tests are forced to include the store tests. Adding the "store"
subsets is just more tests to maintain, understand, and run, with no
better test coverage and virtually no extra diagnosibility. Just keep
the load tests and simplify the test names.
It turns out, these two calls:
dt_cg_alloca_access_check(dlp, drp, dreg, DT_ISIMM, size);
dt_cg_alloca_ptr(dlp, drp, dreg, dreg);
are basically always made in tandem... no surprise. (There is a little
nuance to this claim, but the oddities are very minor.) It makes sense
to combine them into one call, simplifying everywhere these calls are made.
On 8/31/23 12:47, Kris Van Hees via DTrace-devel wrote:
> Storing to and loading from struct or union members of alloca pointers
> was not properly turning the alloca pointer into a real pointer, nor was
> it performing boundary checks.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> ---
> libdtrace/dt_cg.c | 32 ++++++++++-----
> .../funcs/alloca/tst.alloca-array-load.d | 29 ++++++++++++++
> .../funcs/alloca/tst.alloca-array-store.d | 28 +++++++++++++
> .../funcs/alloca/tst.alloca-deref-load.d | 29 ++++++++++++++
> .../funcs/alloca/tst.alloca-deref-store.d | 28 +++++++++++++
> .../funcs/alloca/tst.alloca-struct-load.d | 33 +++++++++++++++
> .../funcs/alloca/tst.alloca-struct-store.d | 32 +++++++++++++++
> .../alloca/tst.alloca-struct-union-load.d | 40 +++++++++++++++++++
> .../alloca/tst.alloca-struct-union-store.d | 39 ++++++++++++++++++
> 9 files changed, 281 insertions(+), 9 deletions(-)
> create mode 100644 test/unittest/funcs/alloca/tst.alloca-array-load.d
> create mode 100644 test/unittest/funcs/alloca/tst.alloca-array-store.d
> create mode 100644 test/unittest/funcs/alloca/tst.alloca-deref-load.d
> create mode 100644 test/unittest/funcs/alloca/tst.alloca-deref-store.d
> create mode 100644 test/unittest/funcs/alloca/tst.alloca-struct-load.d
> create mode 100644 test/unittest/funcs/alloca/tst.alloca-struct-store.d
> create mode 100644 test/unittest/funcs/alloca/tst.alloca-struct-union-load.d
> create mode 100644 test/unittest/funcs/alloca/tst.alloca-struct-union-store.d
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 5c7e3998..4e163e01 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -3051,7 +3051,7 @@ dt_cg_store(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dst)
> int dreg = dst->dn_reg;
>
> /*
> - * If we're loading a bit-field, the size of our store is found by
> + * If we're storing into a bit-field, the size of our store is found by
> * rounding dst's cte_bits up to a byte boundary and then finding the
> * nearest power of two to this value (see clp2(), above).
> */
> @@ -3062,13 +3062,15 @@ dt_cg_store(dt_node_t *src, dt_irlist_t *dlp, dt_regset_t *drp, dt_node_t *dst)
> size = dt_node_type_size(dst);
>
> /*
> - * If we're loading a writable non-alloca lvalue, and it's a
> - * dereference, and *its* child is an alloca pointer, then this is a
> - * dereferenced alloca pointer and needs bounds-checking (which could
> - * not be done at deref time due to not knowing the size of the write).
> + * If we're storing into a writable lvalue that is a dereference of an
> + * alloca pointer or if we are storing into a writable alloca lvalue,
> + * we need to do bounds-checking and turn the offset value into a real
> + * pointer.
> */
> - if (dst->dn_flags & DT_NF_WRITABLE && dst->dn_flags & DT_NF_LVALUE
> - && dst->dn_op == DT_TOK_DEREF && dst->dn_child->dn_flags & DT_NF_ALLOCA) {
> + if (dst->dn_flags & DT_NF_WRITABLE && dst->dn_flags & DT_NF_LVALUE &&
> + ((dst->dn_op == DT_TOK_DEREF &&
> + dst->dn_child->dn_flags & DT_NF_ALLOCA) ||
> + dst->dn_flags & DT_NF_ALLOCA)) {
> assert(!(dst->dn_flags & DT_NF_BITFIELD));
>
> if ((dreg = dt_regset_alloc(drp)) == -1)
> @@ -6473,7 +6475,7 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
> }
>
> if (m.ctm_offset != 0)
> - emit(dlp, BPF_ALU64_IMM(BPF_ADD, dnp->dn_left->dn_reg, m.ctm_offset / NBBY));
> + emit(dlp, BPF_ALU64_IMM(BPF_ADD, dnp->dn_reg, m.ctm_offset / NBBY));
>
> if (!(dnp->dn_flags & DT_NF_REF)) {
> uint_t op;
> @@ -6481,8 +6483,20 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>
> op = dt_cg_ldsize(dnp, ctfp, m.ctm_type, &size);
>
> + /*
> + * If the left-hand side of PTR or DOT is an alloca
> + * pointer, bounds-check the pointer reference.
> + */
> + if (dnp->dn_left->dn_flags & DT_NF_ALLOCA) {
> + assert(dnp->dn_flags & DT_NF_ALLOCA);
> + dt_cg_alloca_access_check(dlp, drp, dnp->dn_reg,
> + DT_ISIMM, size);
> + dt_cg_alloca_ptr(dlp, drp, dnp->dn_reg,
> + dnp->dn_reg);
> + }
> +
> if (dnp->dn_left->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))
> - emit(dlp, BPF_LOAD(op, dnp->dn_left->dn_reg, dnp->dn_left->dn_reg, 0));
> + emit(dlp, BPF_LOAD(op, dnp->dn_reg, dnp->dn_reg, 0));
> else
> dt_cg_load_scalar(dnp->dn_left, op, size, dlp, drp);
> }
> diff --git a/test/unittest/funcs/alloca/tst.alloca-array-load.d b/test/unittest/funcs/alloca/tst.alloca-array-load.d
> new file mode 100644
> index 00000000..3db98049
> --- /dev/null
> +++ b/test/unittest/funcs/alloca/tst.alloca-array-load.d
> @@ -0,0 +1,29 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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: Loading from array elements works for alloca pointers.
> + *
> + * SECTION: Actions and Subroutines/alloca()
> + */
> +
> +#pragma D option quiet
> +
> +int *arr;
> +
> +BEGIN
> +{
> + arr = alloca(5 * sizeof(int));
> + arr[3] = 0x42;
> + trace(arr[3]);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/alloca/tst.alloca-array-store.d b/test/unittest/funcs/alloca/tst.alloca-array-store.d
> new file mode 100644
> index 00000000..17edb584
> --- /dev/null
> +++ b/test/unittest/funcs/alloca/tst.alloca-array-store.d
> @@ -0,0 +1,28 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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: Storing to array elements works for alloca pointers.
> + *
> + * SECTION: Actions and Subroutines/alloca()
> + */
> +
> +#pragma D option quiet
> +
> +int *arr;
> +
> +BEGIN
> +{
> + arr = alloca(5 * sizeof(int));
> + arr[3] = 0x42;
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/alloca/tst.alloca-deref-load.d b/test/unittest/funcs/alloca/tst.alloca-deref-load.d
> new file mode 100644
> index 00000000..2d23cdd3
> --- /dev/null
> +++ b/test/unittest/funcs/alloca/tst.alloca-deref-load.d
> @@ -0,0 +1,29 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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: Loading from dereferenced alloca pointers works.
> + *
> + * SECTION: Actions and Subroutines/alloca()
> + */
> +
> +#pragma D option quiet
> +
> +int *val;
> +
> +BEGIN
> +{
> + val = alloca(sizeof(int));
> + *val = 0x42;
> + trace(*val);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/alloca/tst.alloca-deref-store.d b/test/unittest/funcs/alloca/tst.alloca-deref-store.d
> new file mode 100644
> index 00000000..e7d36e53
> --- /dev/null
> +++ b/test/unittest/funcs/alloca/tst.alloca-deref-store.d
> @@ -0,0 +1,28 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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: Storing to dereferenced alloca pointers works.
> + *
> + * SECTION: Actions and Subroutines/alloca()
> + */
> +
> +#pragma D option quiet
> +
> +int *val;
> +
> +BEGIN
> +{
> + val = alloca(sizeof(int));
> + *val = 0x42;
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/alloca/tst.alloca-struct-load.d b/test/unittest/funcs/alloca/tst.alloca-struct-load.d
> new file mode 100644
> index 00000000..5f022985
> --- /dev/null
> +++ b/test/unittest/funcs/alloca/tst.alloca-struct-load.d
> @@ -0,0 +1,33 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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: Loading from struct members works for alloca pointers.
> + *
> + * SECTION: Actions and Subroutines/alloca()
> + */
> +
> +#pragma D option quiet
> +
> +struct foo {
> + int a;
> + char b;
> + int c;
> +} *ptr;
> +
> +BEGIN
> +{
> + ptr = alloca(sizeof(struct foo));
> + ptr->c = 0x42;
> + trace(ptr->c);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/alloca/tst.alloca-struct-store.d b/test/unittest/funcs/alloca/tst.alloca-struct-store.d
> new file mode 100644
> index 00000000..4f55f958
> --- /dev/null
> +++ b/test/unittest/funcs/alloca/tst.alloca-struct-store.d
> @@ -0,0 +1,32 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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: Storing to struct members works for alloca pointers.
> + *
> + * SECTION: Actions and Subroutines/alloca()
> + */
> +
> +#pragma D option quiet
> +
> +struct foo {
> + int a;
> + char b;
> + int c;
> +} *ptr;
> +
> +BEGIN
> +{
> + ptr = alloca(sizeof(struct foo));
> + ptr->c = 0x42;
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/alloca/tst.alloca-struct-union-load.d b/test/unittest/funcs/alloca/tst.alloca-struct-union-load.d
> new file mode 100644
> index 00000000..7361b494
> --- /dev/null
> +++ b/test/unittest/funcs/alloca/tst.alloca-struct-union-load.d
> @@ -0,0 +1,40 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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: Loading from nested members works for alloca pointers.
> + *
> + * SECTION: Actions and Subroutines/alloca()
> + */
> +
> +#pragma D option quiet
> +
> +struct st {
> + struct {
> + union {
> + struct {
> + int a;
> + char b;
> + int c;
> + } apa;
> + uint64_t val;
> + } u;
> + } s;
> +} *ptr;
> +
> +BEGIN
> +{
> + ptr = alloca(sizeof(struct st));
> + ptr->s.u.apa.c = 0x42;
> + trace(ptr->s.u.apa.c);
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
> diff --git a/test/unittest/funcs/alloca/tst.alloca-struct-union-store.d b/test/unittest/funcs/alloca/tst.alloca-struct-union-store.d
> new file mode 100644
> index 00000000..c0cbd787
> --- /dev/null
> +++ b/test/unittest/funcs/alloca/tst.alloca-struct-union-store.d
> @@ -0,0 +1,39 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2023, 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: Storing to nested members works for alloca pointers.
> + *
> + * SECTION: Actions and Subroutines/alloca()
> + */
> +
> +#pragma D option quiet
> +
> +struct st {
> + struct {
> + union {
> + struct {
> + int a;
> + char b;
> + int c;
> + } apa;
> + uint64_t val;
> + } u;
> + } s;
> +} *ptr;
> +
> +BEGIN
> +{
> + ptr = alloca(sizeof(struct st));
> + ptr->s.u.apa.c = 0x42;
> + exit(0);
> +}
> +
> +ERROR
> +{
> + exit(1);
> +}
More information about the DTrace-devel
mailing list