From owner-dev-commits-src-all@freebsd.org Sat Jan 9 02:05:51 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 8E8434DED8C; Sat, 9 Jan 2021 02:05:51 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com [IPv6:2a00:1450:4864:20::42c]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DCNbz3WP4z3sHx; Sat, 9 Jan 2021 02:05:51 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-wr1-x42c.google.com with SMTP id d26so10644130wrb.12; Fri, 08 Jan 2021 18:05:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=Aus77BpKwLdqksQ3tLicJMVnZh/aYcMTLGCobYuYR58=; b=WOtP1/Mtgr2nqrR13rMJ4znyH/zDTt2X/vyN7KfpYNiRZxk8vqPoLHBjZ3P2JPgOjO DSFiP0G1Mj0AmYinoZkXQ4HxAXp2NBgOBCeEyQr5cFR0b2xR2czDVLfKazHHIVDlaswh amemKNhlEgMg6AuOR3QVAWRk7c4r9MYf89Sy5u7cr4EdCiVk/HBtDNnEu62xtyGKgmOA moaJc8kpztOLnnriGizETLgx2iWojKw3KelodD1vo2hIgvE3/6MMiHa+S92ZhG5x579u NlgSG8gRj5BfrH6k0sPdKNy3vqjZSnGmXUb0EvyyKuKlL0as/JmQ1mMOuEGiZSerIOAr rmDQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=Aus77BpKwLdqksQ3tLicJMVnZh/aYcMTLGCobYuYR58=; b=opwYvFs26sh6m0JFu3WMmMLReJYQc4FQWsmt48Y2PVQRWmWsHZQjXclgBBqfJHaE1p /hGneYQrO+8UvwL68TAydXX6GHjXugT6kvgJsz7pDJMqim7gPrRRBdZFQwaYHwiBGs9h pAksgcUjfSeDnmkKO6T+fsVKN4TzlFgNt8fg8wJYbkH0piNsETDk6kTeEK1gZvXNJyPG Us8VZTtTwNFSgWzdi4ATftZj1+FEkfh03iuM/FRdc7wv0YstNHOlWbl8eLeGd44vo3jw eZBHqulN7J5r5UnWEvDZ2cZyfiEsgxVsXK43n22zU3OZYsp9aV/FgazgwdEln0y7Y+VM vOoQ== X-Gm-Message-State: AOAM5320+8PH5TGxHKf7WYkrEstirArZoQTxNbHNqT/rbO6MeXU+36ym UUvDyMSoEzSHYNiFEHZQ0KJpTxUkPHj/FvPI+flbQeJC3To= X-Google-Smtp-Source: ABdhPJyoTjAHJ7q/4SdshmgwwSyMrIEnXGDNilwGLHXHJCPcBUwYLeaHFEAGd0XyIggtwbXvlt37YC3uddD1U6uhr6s= X-Received: by 2002:adf:ec86:: with SMTP id z6mr6103136wrn.17.1610157949607; Fri, 08 Jan 2021 18:05:49 -0800 (PST) MIME-Version: 1.0 Received: by 2002:adf:f811:0:0:0:0:0 with HTTP; Fri, 8 Jan 2021 18:05:48 -0800 (PST) In-Reply-To: <202101082237.108MbjU1068611@gitrepo.freebsd.org> References: <202101082237.108MbjU1068611@gitrepo.freebsd.org> From: Mateusz Guzik Date: Sat, 9 Jan 2021 03:05:48 +0100 Message-ID: Subject: Re: git: f222a6b88614 - main - dtrace: Fix /"string" == NULL/ comparisons using an uninitialized value. To: Bryan Drewery Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 4DCNbz3WP4z3sHx X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 09 Jan 2021 02:05:51 -0000 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 wrote: > The branch main has been updated by bdrewery: > > URL: > https://cgit.FreeBSD.org/src/commit/?id=f222a6b88614db13ae83c8110281e690d1381a4c > > commit f222a6b88614db13ae83c8110281e690d1381a4c > Author: Bryan Drewery > AuthorDate: 2020-12-18 17:58:03 +0000 > Commit: Bryan Drewery > 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 > 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