From nobody Tue Dec 16 21:58:16 2025 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4dW9mF4Ghvz6LGfV; Tue, 16 Dec 2025 21:58:21 +0000 (UTC) (envelope-from fuz@fuz.su) Received: from fuz.su (fuz.su [IPv6:2001:41d0:8:e508::1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "fuz.su", Issuer "fuz.su" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 4dW9mD1RpXz4Jdk; Tue, 16 Dec 2025 21:58:20 +0000 (UTC) (envelope-from fuz@fuz.su) Authentication-Results: mx1.freebsd.org; dkim=none; dmarc=none; spf=pass (mx1.freebsd.org: domain of fuz@fuz.su designates 2001:41d0:8:e508::1 as permitted sender) smtp.mailfrom=fuz@fuz.su Received: from fuz.su (localhost [127.0.0.1]) by fuz.su (8.18.1/8.18.1) with ESMTPS id 5BGLwG7B048193 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NO); Tue, 16 Dec 2025 22:58:16 +0100 (CET) (envelope-from fuz@fuz.su) Received: (from fuz@localhost) by fuz.su (8.18.1/8.18.1/Submit) id 5BGLwGMh048192; Tue, 16 Dec 2025 22:58:16 +0100 (CET) (envelope-from fuz) Date: Tue, 16 Dec 2025 22:58:16 +0100 From: Robert Clausecker To: Ravi Pokala Cc: Cy Schubert , Robert Clausecker , src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: 66eb78377bf1 - main - libc/amd64: fix overread conditions in stpncpy() Message-ID: References: <693ee0f1.3662d.650a5e21@gitrepo.freebsd.org> <20251216135709.46D25203@slippy.cwsent.com> <20251216160002.A65D63D6@slippy.cwsent.com> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-main@freebsd.org Sender: owner-dev-commits-src-main@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-Spamd-Bar: --- X-Spamd-Result: default: False [-3.30 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-1.00)[-0.997]; R_SPF_ALLOW(-0.20)[+a]; MIME_GOOD(-0.10)[text/plain]; ARC_NA(0.00)[]; ASN(0.00)[asn:16276, ipnet:2001:41d0::/32, country:FR]; FREEFALL_USER(0.00)[fuz]; TO_DN_SOME(0.00)[]; MIME_TRACE(0.00)[0:+]; FROM_HAS_DN(0.00)[]; MID_RHS_MATCH_FROM(0.00)[]; R_DKIM_NA(0.00)[]; RCVD_COUNT_TWO(0.00)[2]; FROM_EQ_ENVFROM(0.00)[]; MISSING_XM_UA(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DMARC_NA(0.00)[fuz.su]; RCVD_TLS_LAST(0.00)[]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org,dev-commits-src-main@freebsd.org]; RCPT_COUNT_FIVE(0.00)[6] X-Rspamd-Queue-Id: 4dW9mD1RpXz4Jdk Am Tue, Dec 16, 2025 at 10:54:33PM +0100 schrieb Robert Clausecker: > Hi Ravi, > > The bts (bit test and set) instruction takes a bit index and an integer and > sets the indicated bit in the register. I.e. bts a, b does a |= 1 << b. That was supposed to be "bts a, b does b |= 1 << a" of course (in AT&T syntax). > It also returns the previous value of the bit in the carry flag, but that's > not really important here. R8 is the 64-bit variant of r8d. I switched to > it for this instruction as bts masks its index to the operand size (i.e. to > 5 bits for a range of 0--31 for the 32 bit case). However, due to the > bug fix, the case r8 == 32 is now possible, which led to wrong behaviour > (that is, the least-significant bit is set). This is avoided by switching to > 64-bit operand size, instead setting the 32nd bit. test does not care about > these bits, so 32 bit operand size is fine. Who does care is the tzcnt > instruction further down, and that's probably the bug I added. Will have > to investigate further and then submit a patch. > > I'm sorry for the continued errors. It's a bit of a stressful time for me. > > Yours, > Robert Clausecker > > Am Tue, Dec 16, 2025 at 02:33:32PM -0500 schrieb Ravi Pokala: > > I know ~nothing of assembly, but one thing that seems odd to me is that the 'bts' line > > originally used '%r8d', but the new version uses '%r8'. And yet, the new 'test' line > > uses '%r8d'. Again, knowing nothing about this, it seems to me that the 'bts' and 'test' > > should probably be talking about the same thing...? > > > > -Ravi (rpokala@) > > > > -----Original Message----- > > From: > on behalf of Cy Schubert > > > Reply-To: Cy Schubert > > > Date: Tuesday, December 16, 2025 at 11:00 > > To: Cy Schubert > > > Cc: Robert Clausecker >, >, >, > > > Subject: Re: git: 66eb78377bf1 - main - libc/amd64: fix overread conditions in stpncpy() > > > > > > In message <20251216135709.46D25203@slippy.cwsent.com >, Cy Schubert writes: > > > In message <693ee0f1.3662d.650a5e21@gitrepo.freebsd.org >, Robert Clausecker > > > wri > > > tes: > > > > The branch main has been updated by fuz: > > > > > > > > URL: https://cgit.FreeBSD.org/src/commit/?id=66eb78377bf109af1d9e25626bf254 > > > b4 > > > > 369436ec > > > > > > > > commit 66eb78377bf109af1d9e25626bf254b4369436ec > > > > Author: Robert Clausecker > > > > > AuthorDate: 2025-12-10 20:45:18 +0000 > > > > Commit: Robert Clausecker > > > > > CommitDate: 2025-12-14 16:06:05 +0000 > > > > > > > > libc/amd64: fix overread conditions in stpncpy() > > > > > > > > Due to incorrect unit test design, two overread conditions went > > > > undetected in the amd64 baseline stpncpy() implementation. > > > > For buffers of 1--16 and 32 bytes that do not contain nul bytes > > > > and end exactly at a page boundary, the code would incorrectly > > > > read 16 bytes from the next page, possibly crossing into an > > > > unmapped page and crashing the program. If the next page was > > > > mapped, the code would then proceed with the expected behaviour > > > > of the stpncpy() function. > > > > > > > > Three changes were made to fix the bug: > > > > > > > > - an off-by-one error is fixed in the code deciding whether to > > > > enter the runt case or not, entering it for 0 > > > instead of 0 > > > - in the runt case, the logic to skip reading a second 16-byte > > > > chunk if the buffer ends in the first chunk was fixed to > > > > account for buffers that end at a 16-byte boundary but do not > > > > hold a nul byte. > > > > - in the runt case, the logic to transform the location of the > > > > end of the input buffer into a bit mask was fixed to allow > > > > the case of n==32, which was previously impossible due to the > > > > incorrect logic for entering said case. > > > > > > > > The performance impact should be minimal. > > > > > > > > PR: 291359 > > > > See also: D54169 > > > > Reported by: Collin Funk > > > > > Reviewed by: getz > > > > Approved by: markj (mentor) > > > > MFC after: 1 week > > > > Fixes: 90253d49db09a9b1490c448d05314f3e4bbfa468 (D42519) > > > > Differential Revision: https://reviews.freebsd.org/D54170 > > > > --- > > > > lib/libc/amd64/string/stpncpy.S | 7 ++++--- > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/lib/libc/amd64/string/stpncpy.S b/lib/libc/amd64/string/stpncp > > > y. > > > > S > > > > index 5ce0dd093a9e..df22bb9f0c53 100644 > > > > --- a/lib/libc/amd64/string/stpncpy.S > > > > +++ b/lib/libc/amd64/string/stpncpy.S > > > > @@ -100,7 +100,7 @@ ARCHENTRY(__stpncpy, baseline) > > > > movdqa (%rsi), %xmm0 # load head > > > > and $0xf, %ecx # offset from alignment > > > > mov $-1, %r9d > > > > - lea -32(%rcx), %rax # set up overflow-proof compari > > > > son rdx+rcx<=32 > > > > + lea -33(%rcx), %rax # set up overflow-proof compari > > > > son rdx+rcx<=32 > > > > shl %cl, %r9d # mask of bytes belonging to th > > > > e string > > > > sub %rcx, %rdi # adjust RDI to correspond to R > > > > SI > > > > pxor %xmm1, %xmm1 > > > > @@ -223,8 +223,9 @@ ARCHENTRY(__stpncpy, baseline) > > > > > > > > /* 1--32 bytes to copy, bounce through the stack */ > > > > .Lrunt: movdqa %xmm1, bounce+16(%rsp) # clear out rest of on- > > > > stack copy > > > > - bts %r10d, %r8d # treat end of buffer as end of > > > > string > > > > - and %r9w, %r8w # end of string within first bu > > > > ffer? > > > > + bts %r10, %r8 # treat end of buffer as end of > > > > string > > > > + and %r9d, %r8d # mask out head before string > > > > + test $0x1ffff, %r8d # end of string within first ch > > > > unk or right after? > > > > jnz 0f # if yes, do not inspect second > > > > buffer > > > > > > > > movdqa 16(%rsi), %xmm0 # load second chunk of input > > > > > > > > > > I've opened PR/291720 regarding a significant regression caused by this > > > commit. It affects my older machines, resulting in enviornment (getenv) > > > corruption. It does not affect my newer (and with more RAM) machines. > > > > > > BTW, this brought my postfix server effectively offline. I have not been > > able to send or receive email until this commit was reverted locally. > > > > > > > > > > -- > > Cheers, > > Cy Schubert > > > FreeBSD UNIX: > Web: https://FreeBSD.org > > NTP: > Web: https://nwtime.org > > > > > > e**(i*pi)+1=0 > > > > > > > > > > > > > > > > > > > > > > > > -- > () ascii ribbon campaign - for an encoding-agnostic world > /\ - against html email - against proprietary attachments -- () ascii ribbon campaign - for an encoding-agnostic world /\ - against html email - against proprietary attachments