[DTrace-devel] [PATCH] cg: lift restriction of 1st arg to bcopy() not being alloca

[Eugene Loh]

Reviewed-by: Eugene Loh <eugene.loh at oracle.com>
with comments below...

To start:

*)  Does this patch modify behavior of copyinto() as well?  If so, 
shouldn't that be mentioned and tested?

*)  Modified files (including dt_cg.c) should have 2023 copyright.

On 1/4/23 01:32, Kris Van Hees via DTrace-devel wrote:
> The documentation states that the source address is not allowed to be
> within the scratch memory region, but the legacy implementation does
> not actually enforce that restriction, nor does it seem necessary.

I've lost track, but have we standardized on what "legacy" means in this 
context?  Will future developers know what this means?  Also, isn't 
Solaris the more compelling precedent?

> This patch also moves bcopy tests into their own directory.
>
> Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
>
> diff --git a/libdtrace/dt_cg.c b/libdtrace/dt_cg.c
> @@ -4411,10 +4411,7 @@ dt_cg_subr_bcopy_impl(dt_node_t *dnp, dt_node_t *dst, dt_node_t *src,
>   			"\t argument: non-alloca pointer\n",
>   			is_bcopy ? "bcopy" : "copyinto", is_bcopy ? 2 : 3);
>   	/* The dst will be NULL-checked in the alloca access check below. */
> -
> -	dt_cg_alloca_access_check(dlp, drp, dst->dn_reg,
> -				  DT_ISREG, size->dn_reg);
> -	dt_cg_alloca_ptr(dlp, drp, dst->dn_reg, dst->dn_reg);
> +	dt_cg_check_ptr_arg(dlp, drp, dst, size);
>   
>   	if (dt_regset_xalloc_args(drp) == -1)
>   		longjmp(yypcb->pcb_jmpbuf, EDT_NOREG);

Should that comment also be removed, since the alloca_access_check() 
call is removed?

> diff --git a/test/unittest/funcs/bcopy/err.badbcopy7.d b/test/unittest/funcs/bcopy/err.badbcopy7.d
> new file mode 100644
> index 00000000..c8fffea5
> --- /dev/null
> +++ b/test/unittest/funcs/bcopy/err.badbcopy7.d
> @@ -0,0 +1,29 @@
> +/*
> + * Oracle Linux DTrace.
> + * Copyright (c) 2006, 2022, Oracle and/or its affiliates. All rights reserved.

2023?  And drop the 2006.  Whatever file this was cut and pasted from 
surely didn't contribute much to this test.  After all, as far as git is 
concerned, this is a new test.

> + * Licensed under the Universal Permissive License v 1.0 as shown at
> + * http://oss.oracle.com/licenses/upl.
> + */
> +
> +/*
> + * ASSERTION: bcopy from an alloca pointer is subject to size checks
> + *
> + * SECTION: Actions and Subroutines/alloca();
> + * 	Actions and Subroutines/bcopy()
> + *

Stylistic consistency would probably drop that blank comment line.

> + */
> +
> +#pragma D option quiet
> +
> +BEGIN
> +{
> +	d = alloca(20);
> +	s = alloca(10);
> +	bcopy(s, d, 20);

Not a big deal, but s and d might as well be src and dst.

> +	exit(0);
> +}
> +
> +ERROR
> +{
> +	exit(1);
> +}
> diff --git a/test/unittest/funcs/err.badbcopy8.d b/test/unittest/funcs/bcopy/err.badbcopy8.d
> similarity index 100%
> rename from test/unittest/funcs/err.badbcopy8.d
> rename to test/unittest/funcs/bcopy/err.badbcopy8.d

Okay, so err.badbcopy8.d is being used "as is," but shouldn't it get 
some scrutiny?  It has:
         ptr = alloca(20);
         /* Attempt to bcopy to scratch memory that isn't allocated,
            with a max exceeding the verifier-checked bound of
            2*scratchsize.  */
         bcopy((void *)&`max_pfn, ptr, 2048000);
But the scratch memory clearly *IS* allocated.  If the point is the size 
is too large, then compared to what?  Apparently, larger than the 
allocated 20 is just fine.  Maybe even up to scratchsize is fine.  Over 
1*scratchsize is not okay, despite the comment.  And testing with 
2048000 seems just plain irrelevant.

> diff --git a/test/unittest/funcs/err.badbcopy7.d b/test/unittest/funcs/bcopy/tst.bcopy2.d
> similarity index 65%
> rename from test/unittest/funcs/err.badbcopy7.d
> rename to test/unittest/funcs/bcopy/tst.bcopy2.d
> index d7390122..6fa01d1c 100644
> --- a/test/unittest/funcs/err.badbcopy7.d
> +++ b/test/unittest/funcs/bcopy/tst.bcopy2.d
> @@ -6,8 +6,8 @@
>    */
>   
>   /*
> - * ASSERTION:
> - *	bcopy should not copy when the source is scratch space
> + * ASSERTION: bcopy to scratch space is allowed (even though documentation
> + *	      claims that it is not)
>    *
>    * SECTION: Actions and Subroutines/alloca();
>    * 	Actions and Subroutines/bcopy()
> @@ -16,14 +16,13 @@
>   
>   #pragma D option quiet
>   
> -
>   BEGIN
>   {
> -	ptr = alloca(sizeof(unsigned long));
> +	ptr = (unsigned long *)alloca(sizeof(unsigned long));
>   	bcopy((void *)&`max_pfn, ptr, sizeof(unsigned long));
> -	ptr2 = alloca(sizeof(unsigned long));
> +	ptr2 = (unsigned long *)alloca(sizeof(unsigned long));
>   	bcopy(ptr, ptr2, sizeof(unsigned long));
> -	exit(0);
> +	exit(*ptr == *ptr2 ? 0 : 1);
>   }
>   
>   ERROR

I realize that this test is inherited from past work, but using 
bcopy(&`max_pfn,...) to initialize *ptr unnecessarily complicates this 
rather simple test.  Why not just *ptr=0x12345678, or something?  That 
makes clearer what's going on.  It'd also be clearer to use "src" and 
"dst" rather than "ptr" and "ptr2".