From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2e.google.com (mail-oa1-x2e.google.com [IPv6:2001:4860:4864:20::2e]) by sourceware.org (Postfix) with ESMTPS id 5EAE83858D33 for ; Tue, 31 Jan 2023 23:40:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5EAE83858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com Received: by mail-oa1-x2e.google.com with SMTP id 586e51a60fabf-163adf0cfc4so10246759fac.7 for ; Tue, 31 Jan 2023 15:40:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=8xoQlQs3AtxHXuxHGVl6j2+w3Moeb5qVxgfJxJEFJfs=; b=PtteGfm8Wyy2hI4az27pl3j0RzMaR7kMHIPbP7IuD013E7H5Mz+IIYzFG4sDMfYC/7 3+NTz1smQ0yMQkoxQuJi8eqWr4GHz0ypOQ6SJklhUaQbouxOXxxg+2knasEN+yfIMyFA jxXwpp8wy88SNAh3r8aQ/cnuCz0LFGmN2YjdKRBKf54xdCGdrYFfF40AIIfR6aROk0JD /BE6OrM540iR2X0NnAKV3+6zdkA7oR2Sy6yG8jqwI6mdEiMrNKydsu16FC6oHAv9wbVd 5NxTv8qZGOrTYp9l/uZcPO9dCI6uGBKlcPKGD9rbmDKgHrJ7GeeRWTO9UQaj3C2GewP0 Mg6g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=8xoQlQs3AtxHXuxHGVl6j2+w3Moeb5qVxgfJxJEFJfs=; b=BcI63eJdTRP06eqOp/Cxr2vxHI3OGyVk1TyfdeuFZqhbkHV/jQoN3BSMJO3BXrAalg sKKDnEu/VLee0jnn7326dAux9e0+M/Kv2Wdv1Nt9B6LMKCXnC1zAxcL2eHtYA5V/0ndo 20GEvfLRIqSltcbcHMf+FfXR6IrveQnFC9qgkYi0XNUb/krSiyS0ODrGqXIRL9HnLyvU pS1A2OiSQQC+HiVxunMx8jFRTeICqd0dat3uxOqz2jU3B4ipnmeyD8pMZwbAYhXykx0C ode8Tm9bOJNl846bB4LZ+fyzWvXWxrQGkQFSeNTSZpzzlo8gUbOhnYr0dTB07qZWruSU DkRw== X-Gm-Message-State: AO0yUKXeUtZwiPJJeV90s8/nB1IUzy1Y7uuA6xqTkwDevswTU9ggaCfV eIPy28aDWRhLJRyHIbyj0AaoODH9x7fKuzuwzVan5sy4Bgo= X-Google-Smtp-Source: AK7set9DNLKxH77i+gzF09jI8lXwA9ZUHwmfFUxVg0tbU5Qr8ai2Rd5aD9h4tcPbW52asuhXsauCjbpPrCj6Q5Eleh0= X-Received: by 2002:a05:6870:41d1:b0:163:9c96:7662 with SMTP id z17-20020a05687041d100b001639c967662mr1208901oac.266.1675208442255; Tue, 31 Jan 2023 15:40:42 -0800 (PST) MIME-Version: 1.0 References: <20230131213655.4033602-1-goldstein.w.n@gmail.com> In-Reply-To: From: "H.J. Lu" Date: Tue, 31 Jan 2023 15:40:06 -0800 Message-ID: Subject: Re: [PATCH v1] x86: Fix strncat-avx2.S when `src` has no null-term [BZ #30065] To: Noah Goldstein Cc: libc-alpha@sourceware.org, carlos@systemhalted.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3022.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, Jan 31, 2023 at 3:09 PM Noah Goldstein wrote: > > On Tue, Jan 31, 2023 at 5:02 PM H.J. Lu wrote: > > > > On Tue, Jan 31, 2023 at 2:52 PM Noah Goldstein wrote: > > > > > > On Tue, Jan 31, 2023 at 4:43 PM H.J. Lu wrote: > > > > > > > > On Tue, Jan 31, 2023 at 2:37 PM Noah Goldstein wrote: > > > > > > > > > > On Tue, Jan 31, 2023 at 4:29 PM H.J. Lu wrote: > > > > > > > > > > > > On Tue, Jan 31, 2023 at 1:37 PM Noah Goldstein wrote: > > > > > > > > > > > > > > Two issue: > > > > > > > > > > > > > > 1) Zero-length check is doing: > > > > > > > ``` > > > > > > > test %rdx, %rdx > > > > > > > jl L(zero_len) > > > > > > > ``` > > > > > > > which doesn't actually check zero (was at some point `decq` and the > > > > > > > flag never got updated). > > > > > > > > > > > > > > The fix is just make the flag `jle` i.e: > > > > > > > ``` > > > > > > > test %rdx, %rdx > > > > > > > jle L(zero_len) > > > > > > > ``` > > > > > > > > > > > > > > 2) Length check in page-cross case checking if we should continue is > > > > > > > doing: > > > > > > > ``` > > > > > > > cmpq %r8, %rdx > > > > > > > jb L(page_cross_small) > > > > > > > ``` > > > > > > > which means we will continue searching for null-term if length ends at > > > > > > > the end of a page and there was no null-term in `src`. > > > > > > > > > > > > It is not purely about null-term. strncat shouldn't read beyond the limit. > > > > > > In this case, src may point to PROT_NONE memory. > > > > > > > > > > > > > > > > If there was a null-term the later check: > > > > > ``` > > > > > shll $CHAR_SIZE, %ecx > > > > > jz L(page_cross_continue) > > > > > ``` > > > > > would catch it, this can only occur if there is no null-term. > > > > > > > > But it is incorrect to read beyond the limit even if there is a null-term. > > > It's a page-aligned read where len != 0 so the original read should be fine. > > > > > > The `jb L(page_cross_small)` > > > falls through to the null-term check, so if there was a null-term we won't > > > read anymore bytes. > > > > There is no need to search null-term for zero length case. > > > This isn't the zero length case, this is the page-cross case. Length > is != 0 here. In the zero length case: test %rdx, %rdx jl L(zero_len) it crashes when src points to an unreadable memory and the limit is 0. strncat shouldn't read beyond the limit. The change is OK. But the commit log should mention "avoid reading beyond the limit." > > > > > > > > > > > The fix is to make the flag: > > > > > > > ``` > > > > > > > cmpq %r8, %rdx > > > > > > > jbe L(page_cross_small) > > > > > > > ``` > > > > > > > --- > > > > > > > string/test-strncat.c | 25 ++++++++++++++++++++++++- > > > > > > > sysdeps/x86_64/multiarch/strncat-avx2.S | 4 ++-- > > > > > > > 2 files changed, 26 insertions(+), 3 deletions(-) > > > > > > > > > > > > > > diff --git a/string/test-strncat.c b/string/test-strncat.c > > > > > > > index e03d329e1c..c0cde206ee 100644 > > > > > > > --- a/string/test-strncat.c > > > > > > > +++ b/string/test-strncat.c > > > > > > > @@ -28,6 +28,7 @@ > > > > > > > # define CHAR char > > > > > > > # define UCHAR unsigned char > > > > > > > # define SIMPLE_STRNCAT simple_strncat > > > > > > > +# define STRNLEN strnlen > > > > > > > # define STRLEN strlen > > > > > > > # define MEMSET memset > > > > > > > # define MEMCPY memcpy > > > > > > > @@ -40,6 +41,7 @@ > > > > > > > # define CHAR wchar_t > > > > > > > # define UCHAR wchar_t > > > > > > > # define SIMPLE_STRNCAT simple_wcsncat > > > > > > > +# define STRNLEN wcsnlen > > > > > > > # define STRLEN wcslen > > > > > > > # define MEMSET wmemset > > > > > > > # define MEMCPY wmemcpy > > > > > > > @@ -78,7 +80,7 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n) > > > > > > > return; > > > > > > > } > > > > > > > > > > > > > > - size_t len = STRLEN (src); > > > > > > > + size_t len = STRNLEN (src, n); > > > > > > > if (MEMCMP (dst + k, src, len + 1 > n ? n : len + 1) != 0) > > > > > > > > > > > > > { > > > > > > > error (0, 0, "Incorrect concatenation in function %s", > > > > > > > @@ -95,6 +97,26 @@ do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t n) > > > > > > > } > > > > > > > } > > > > > > > > > > > > > > +static void > > > > > > > +do_test_src_no_nullterm_bz30065 (void) > > > > > > > +{ > > > > > > > + /* NB: "src does not need to be null-terminated if it contains n or more > > > > > > > + * bytes." */ > > > > > > > + CHAR *s1, *s2; > > > > > > > + size_t bound = page_size / sizeof (CHAR); > > > > > > > + s1 = (CHAR *) (buf1 + BUF1PAGES * page_size); > > > > > > > + s2 = (CHAR *) buf2; > > > > > > > + MEMSET (s1 - bound, -1, bound); > > > > > > > + for (size_t n = 0; n < bound; ++n) > > > > > > > + { > > > > > > > + FOR_EACH_IMPL (impl, 0) > > > > > > > + { > > > > > > > + s2[0] = '\0'; > > > > > > > + do_one_test (impl, s2, s1 - n, n); > > > > > > > + } > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > static void > > > > > > > do_test (size_t align1, size_t align2, size_t len1, size_t len2, > > > > > > > size_t n, int max_char) > > > > > > > @@ -372,6 +394,7 @@ test_main (void) > > > > > > > > > > > > > > do_random_tests (); > > > > > > > do_overflow_tests (); > > > > > > > + do_test_src_no_nullterm_bz30065 (); > > > > > > > return ret; > > > > > > > } > > > > > > > > > > > > > > diff --git a/sysdeps/x86_64/multiarch/strncat-avx2.S b/sysdeps/x86_64/multiarch/strncat-avx2.S > > > > > > > index b380e8e11c..c2ff202238 100644 > > > > > > > --- a/sysdeps/x86_64/multiarch/strncat-avx2.S > > > > > > > +++ b/sysdeps/x86_64/multiarch/strncat-avx2.S > > > > > > > @@ -66,7 +66,7 @@ ENTRY(STRNCAT) > > > > > > > salq $2, %rdx > > > > > > > # else > > > > > > > test %rdx, %rdx > > > > > > > - jl L(zero_len) > > > > > > > + jle L(zero_len) > > > > > > > # endif > > > > > > > vpxor %VZERO_128, %VZERO_128, %VZERO_128 > > > > > > > > > > > > > > @@ -387,7 +387,7 @@ L(page_cross): > > > > > > > subl %esi, %r8d > > > > > > > andl $(VEC_SIZE - 1), %r8d > > > > > > > cmpq %r8, %rdx > > > > > > > - jb L(page_cross_small) > > > > > > > + jbe L(page_cross_small) > > > > > > > > > > > > > > /* Optimizing more aggressively for space as this is very cold > > > > > > > code. This saves 2x cache lines. */ > > > > > > > -- > > > > > > > 2.34.1 > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > H.J. > > > > > > > > > > > > > > > > -- > > > > H.J. > > > > > > > > -- > > H.J. -- H.J.