[DTrace-devel] [PATCH 02/14] Replace not-NULL test with more general runtime test

[Kris Van Hees]

On Mon, May 01, 2023 at 11:47:10PM -0400, eugene.loh--- via DTrace-devel wrote:
> From: Eugene Loh <eugene.loh at oracle.com>
> 
> When we dereference a pointer, it should generally be safe to
> dereference D (including alloca) pointers, even if a more robust
> mechanism is needed to guard against dereferencing unsafe pointers
> in the more general case.  We do, however, check whether a pointer
> is NULL.
> 
> There are times, however, when even a D pointer cannot safely be
> dereferenced.  This will especially be true once NULL string pointers
> will be supported.

I am not sure I get this...  Wouldn't a NULL string pointer be NULL, i.e.
when you load a string value and it has the 'magic value' you should make
the value of the string pointer NULL, and from that point on it truly is a
NULL pointer.  Right?

> So drop the NULL-pointer check and implement dereferencing with a
> more general scalar load.

See below why that is a bad idea.

> Signed-off-by: Eugene Loh <eugene.loh at oracle.com>
> ---
>  libdtrace/dt_cg.c                   | 6 +-----
>  test/unittest/pointers/tst.basic2.d | 3 +--
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> index 483603ef..c1fd46c0 100644
> --- a/libdtrace/dt_cg.c
> +++ b/libdtrace/dt_cg.c
> @@ -5884,7 +5884,6 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>  			uint_t	op;
>  			ssize_t	size;
>  
> -			dt_cg_check_notnull(dlp, drp, dnp->dn_reg);
>  			op = dt_cg_ldsize(dnp, ctfp, dnp->dn_type, &size);
>  
>  			/*
> @@ -5899,10 +5898,7 @@ dt_cg_node(dt_node_t *dnp, dt_irlist_t *dlp, dt_regset_t *drp)
>  						 dnp->dn_reg);
>  			}
>  
> -			if (dnp->dn_child->dn_flags & (DT_NF_ALLOCA | DT_NF_DPTR))
> -				emit(dlp, BPF_LOAD(op, dnp->dn_reg, dnp->dn_reg, 0));
> -			else
> -				dt_cg_load_scalar(dnp, op, size, dlp, drp);
> +			dt_cg_load_scalar(dnp, op, size, dlp, drp);

The problem here is that you end up going back to the general case of
treating all pointers here as arbitrary pointers which bypasses the BPF
verifier checks that the load instruction is able to count on.

The main problem you are experiencing is that *assoc[val] is not being
recognized as dereferencing an arbitrary pointer (and it has to be an
arbitrary pointee we cannot store pointers to D managed storage as real
pointers in an associative array.  So, I think this code needs to be updated
to recognize this specific case and not treat assoc[val] as a DPTR just
because assoc is a DPTR.

>  		}
>  		break;
>  
> diff --git a/test/unittest/pointers/tst.basic2.d b/test/unittest/pointers/tst.basic2.d
> index 559fd93b..c17f9251 100644
> --- a/test/unittest/pointers/tst.basic2.d
> +++ b/test/unittest/pointers/tst.basic2.d
> @@ -1,10 +1,9 @@
>  /*
>   * Oracle Linux DTrace.
> - * Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
> + * Copyright (c) 2006, 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.
>   */
> -/* @@xfail: dtv2 */
>  
>  /*
>   * ASSERTION: Pointers can be stored in associative arrays.
> -- 
> 2.18.4
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel