git: f222a6b88614 - main - dtrace: Fix /"string" == NULL/ comparisons using an uninitialized value.

[Mateusz Guzik]

tinderbox fails to build riscv kernels with:

In file included from
/usr/src/sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c:18434:
/usr/src/sys/cddl/dev/dtrace/riscv/dtrace_isa.c:155:13: error:
variable 'oldfp' is uninitialized when used here
[-Werror,-Wuninitialized]
                if (fp == oldfp) {
                          ^~~~~
/usr/src/sys/cddl/dev/dtrace/riscv/dtrace_isa.c:122:17: note:
initialize the variable 'oldfp' to silence this warning
        uintptr_t oldfp;
                       ^
                        = 0
1 error generated.

which stems from the Makefile change.

On 1/8/21, Bryan Drewery <bdrewery at freebsd.org> wrote:
> The branch main has been updated by bdrewery:
>
> URL:
> https://cgit.FreeBSD.org/src/commit/?id=f222a6b88614db13ae83c8110281e690d1381a4c
>
> commit f222a6b88614db13ae83c8110281e690d1381a4c
> Author:     Bryan Drewery <bdrewery at FreeBSD.org>
> AuthorDate: 2020-12-18 17:58:03 +0000
> Commit:     Bryan Drewery <bdrewery at FreeBSD.org>
> CommitDate: 2021-01-08 22:37:17 +0000
>
>     dtrace: Fix /"string" == NULL/ comparisons using an uninitialized
> value.
>
>     A test of this is funcs/tst.strtok.d which has this filter:
>
>         BEGIN
>         /(this->field = strtok(this->str, ",")) == NULL/
>         {
>                 exit(1);
>         }
>     The test will randomly fail with exit status of 1 indicating that
> this->field
>     was NULL even though printing it out shows it is not.
>
>     This is compiled to the DTrace instruction set:
>         // Pushed arguments not shown here
>         // call strtok() and set result into %r1
>         07: 2f001f01    call DIF_SUBR(31), %r1          ! strtok
>         // set thread local scalar this->field from %r1
>         08: 39050101    stls %r1, DT_VAR(1281)          ! DT_VAR(1281) =
> "field"
>         // Prepare for the == comparison
>         // Set right side of %r2 to NULL
>         09: 25000102    setx DT_INTEGER[1], %r2         ! 0x0
>         // string compare %r1 (strtok result) to %r2
>         10: 27010200    scmp %r1, %r2
>
>     In this case only %r1 is loaded with a string limit set to lim1.  %r2
> being
>     NULL does not get loaded and does not set lim2.  Then we call
> dtrace_strncmp()
>     with MIN(lim1, lim2) resulting in passing 0 and comparing neither side.
>     dtrace_strncmp() handles this case fine and it already has been while
>     being lucky with what lim2 was [un]initialized as.
>
>     Reviewed by:    markj, Don Morris <dgmorris AT earthlink.net>
>     Sponsored by:   Dell EMC
>     Differential Revision:  https://reviews.freebsd.org/D27671
> ---
>  sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c | 8 ++++++++
>  sys/modules/dtrace/dtrace/Makefile                      | 1 -
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
> b/sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
> index 3d68a68ba819..b212185a4578 100644
> --- a/sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
> +++ b/sys/cddl/contrib/opensolaris/uts/common/dtrace/dtrace.c
> @@ -6374,6 +6374,14 @@ dtrace_dif_emulate(dtrace_difo_t *difo,
> dtrace_mstate_t *mstate,
>  			uintptr_t s2 = regs[r2];
>  			size_t lim1, lim2;
>
> +			/*
> +			 * If one of the strings is NULL then the limit becomes
> +			 * 0 which compares 0 characters in dtrace_strncmp()
> +			 * resulting in a false positive.  dtrace_strncmp()
> +			 * treats a NULL as an empty 1-char string.
> +			 */
> +			lim1 = lim2 = 1;
> +
>  			if (s1 != 0 &&
>  			    !dtrace_strcanload(s1, sz, &lim1, mstate, vstate))
>  				break;
> diff --git a/sys/modules/dtrace/dtrace/Makefile
> b/sys/modules/dtrace/dtrace/Makefile
> index 80278fc83a32..55f9f4f66f36 100644
> --- a/sys/modules/dtrace/dtrace/Makefile
> +++ b/sys/modules/dtrace/dtrace/Makefile
> @@ -61,6 +61,5 @@ CFLAGS+=	-include
> ${SYSDIR}/cddl/compat/opensolaris/sys/debug_compat.h
>  CFLAGS.dtrace_asm.S+= -D_SYS_ERRNO_H_ -D_SYS_PARAM_H_ -DLOCORE
>  CWARNFLAGS+=	${OPENZFS_CWARNFLAGS}
>  CWARNFLAGS+=	-Wno-parentheses
> -CWARNFLAGS+=	-Wno-uninitialized
>  CWARNFLAGS+=	-Wno-cast-qual
>  CWARNFLAGS+=	-Wno-unused
>


-- 
Mateusz Guzik <mjguzik gmail.com>