From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id 0D0F93858D28 for ; Mon, 17 Jul 2023 18:50:45 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 0D0F93858D28 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1689619844; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=55dING3Qi3LIlCfu9VSMRNCTVRPBHBoJjTGUxSI4lho=; b=jWuYSk17YSN3CV9KGUtAof1Qja6A3tM4kBdRqUtVlqbz/yy9vKOjE5QW7cnI4c7MpQNXKp O1o+L2R3Ki3WYRr3RPAgqAv7P7Qe2k6TnXKop0m+SfghSVd4pPZlgK3c0IDkEv6+5Kxw4p uCDrEA2NvfKEMQVaH7K183sGTk/wWlo= Received: from mail-oo1-f69.google.com (mail-oo1-f69.google.com [209.85.161.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-398-VDAxNVvFMXGu4CBCS4M72A-1; Mon, 17 Jul 2023 14:50:40 -0400 X-MC-Unique: VDAxNVvFMXGu4CBCS4M72A-1 Received: by mail-oo1-f69.google.com with SMTP id 006d021491bc7-5559caee9d3so5509197eaf.2 for ; Mon, 17 Jul 2023 11:50:39 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1689619838; x=1692211838; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=55dING3Qi3LIlCfu9VSMRNCTVRPBHBoJjTGUxSI4lho=; b=AcIAlbbCwBReKa59R+h1Jy8he+uAS7SmC8wouB/3Dm7TRXuPB/9g3JuY1/IndE9icY 7S1qBV5SL6LZDTp0ulVkAGpEs12kQWB8SCZ2fQCL6qM19B9B+FxsWDg0ck3FuWHdJ6ru hSjHlHxFWoXMFFz4lDQd0bo50P9A1z42qvjWeuNUg9S8xT8RXAvo5NckGw5XhtKBzy3Y 1wP0zjsv95ENGdZDUAeQJ0tB2s7GAFi+D4G1Ec50RP8bReFmSHnC9ah5fzWOo1LN/Dsp 9tsSb5PIg35uf4PbNQnNbmY3fD0en1oWH/zATeNhyaB0Ejd7FfIJVbUrAh0sAuvbPdG1 bW0A== X-Gm-Message-State: ABy/qLZNiT95jTzYIHwhIS+EqIVF8QO/zqj30aAh7jlhyJc+bvS62uNj nYbQIDtrgVe2q4DsqMT8Bs24HftbhbWix7ECbj5xRwMNDPAQIE2Itu/NAbkCXf/1EsKzMvr+sZd NEV8BtGVIoThRmQfGYU8R3hyF5fvr X-Received: by 2002:a05:6358:9325:b0:134:c4dc:2c43 with SMTP id x37-20020a056358932500b00134c4dc2c43mr9749595rwa.28.1689619838337; Mon, 17 Jul 2023 11:50:38 -0700 (PDT) X-Google-Smtp-Source: APBJJlHBA090TiE8ZtZkOhAwEe8ePPwu7vmapKz2OXRZn99pkrnpD8YZZzM3vSZlTwDbGeQGpiKqNQ== X-Received: by 2002:a05:6358:9325:b0:134:c4dc:2c43 with SMTP id x37-20020a056358932500b00134c4dc2c43mr9749581rwa.28.1689619837885; Mon, 17 Jul 2023 11:50:37 -0700 (PDT) Received: from [192.168.0.241] ([198.48.244.52]) by smtp.gmail.com with ESMTPSA id e126-20020a0df584000000b00577269ba9e9sm25672ywf.86.2023.07.17.11.50.37 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 17 Jul 2023 11:50:37 -0700 (PDT) Message-ID: <826256cc-281f-999e-0e4f-3cbc0cfbc3ab@redhat.com> Date: Mon, 17 Jul 2023 14:50:36 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Subject: Re: [PATCH] Add strlcpy/wcslcpy testcase To: DJ Delorie , Sunil K Pandey Cc: libc-alpha@sourceware.org, hjl.tools@gmail.com References: From: Carlos O'Donell Organization: Red Hat In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-12.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE 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 7/14/23 21:07, DJ Delorie via Libc-alpha wrote: > Sunil K Pandey via Libc-alpha writes: >> This patch implements comprehensive tests for strlcpy/wcslcpy >> functions. Tests are mostly derived from strncpy test suites >> and modified to incorporate strlcpy/wcslcpy specifications. > > > Looks OK to me, but the SIMPLE_STRLCPY implementation is incorrect. > However, it's also unused. What is its purpose? > >> + test-strlcpy \ > We should avoid test-strlcpy and tst-strlcpy, and instead rename this to tst-strclpy2, or some other name. We currently have test-* and tst-* overlap for in string/* it is for strlen. Please update with a v2 given DJ's comments. Test cases like this can be included without problem in the release (unless they fail). > >> +/* Test strlcpy functions. >> + Copyright (C) 2023 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + . */ > > Ok. > >> +#ifdef WIDE >> +# include >> +# define CHAR wchar_t >> +# define BIG_CHAR WCHAR_MAX >> +# define SMALL_CHAR 1273 >> +# define MEMCMP wmemcmp >> +# define MEMSET wmemset >> +# define STRLEN wcslen >> +#else >> +# define CHAR char >> +# define BIG_CHAR CHAR_MAX >> +# define SMALL_CHAR 127 >> +# define MEMCMP memcmp >> +# define MEMSET memset >> +# define STRLEN strlen >> +#endif /* !WIDE */ >> + >> + >> +#ifndef SIMPLE_STRLCPY >> +# define TEST_MAIN >> +# ifndef WIDE >> +# define TEST_NAME "strlcpy" >> +# else >> +# define TEST_NAME "wcslcpy" >> +# endif /* WIDE */ >> +# include "test-string.h" >> +# ifndef WIDE >> +# define SIMPLE_STRLCPY simple_strlcpy >> +# define STRLCPY strlcpy >> +# else >> +# define SIMPLE_STRLCPY simple_wcslcpy >> +# define STRLCPY wcslcpy >> +# endif /* WIDE */ > > Consistent with other tests, so OK. > >> +IMPL (STRLCPY, 1) > > Ok. > >> +/* Naive implementation to verify results. */ >> +size_t >> +SIMPLE_STRLCPY (CHAR *dst, const CHAR *src, size_t n) >> +{ >> + size_t ret = STRLEN (src); >> + while (n--) >> + if ((*dst++ = *src++) == '\0') >> + return ret; >> + *dst = '\0'; >> + return ret; >> +} > > This is wrong. It writes N+1 bytes to DST when SRC is long enough to. > > Also, I don't see this used anywhere, including digging through other > tests to see if maybe there's magic involved. Or, I removed it from the > test and the test still passed, so... why? > >> +#endif /* !SIMPLE_STRLCPY */ >> + >> +typedef size_t (*proto_t) (CHAR *, const CHAR *, size_t); >> + >> +static void >> +do_one_test (impl_t *impl, CHAR *dst, const CHAR *src, size_t len, size_t n) >> +{ >> + if (CALL (impl, dst, src, n) != len) >> + { >> + error (0, 0, "Wrong result in function %s %zd %zd", impl->name, >> + CALL (impl, dst, src, n), len); >> + ret = 1; >> + return; >> + } > > Verifying we return the expected value. Ok. > > >> + if (n == 0) >> + return; > > No buffer, nothing further to check. Ok. > >> + len = (len >= n ? n - 1 : len); >> + if (MEMCMP (dst, src, len) != 0) >> + { >> + error (0, 0, "Wrong result in function1 %s", impl->name); >> + ret = 1; >> + return; >> + } > > LEN is strlen(src) (does not include NUL) > N is buffer size (includes NUL) > > If LEN >= N the string won't fit; at most N-1 bytes match > if LEN < N the string will fit; at most LEN+1 bytes match (including the > NUL) > > the memcmp is the lesser of these, OK. > >> + if (dst [len] != '\0') >> + { >> + error (0, 0, "Wrong result in function2 %s", impl->name); >> + ret = 1; >> + return; >> + } >> +} > > Ok. > >> +static void >> +do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char) >> +{ >> + size_t i; >> + CHAR *s1, *s2; >> + >> + /* For wcslcpy: align1 and align2 here mean alignment not in bytes, >> + but in wchar_ts, in bytes it will equal to align * (sizeof (wchar_t)). */ >> + align1 &= 7; >> + if ((align1 + len) * sizeof (CHAR) >= page_size) >> + return; >> + >> + align2 &= 7; >> + if ((align2 + len) * sizeof (CHAR) >= page_size) >> + return; >> + >> + s1 = (CHAR *) (buf1) + align1; >> + s2 = (CHAR *) (buf2) + align2; >> + >> + for (i = 0; i < len; ++i) >> + s1[i] = 32 + 23 * i % (max_char - 32); >> + s1[len] = 0; >> + >> + FOR_EACH_IMPL (impl, 0) >> + do_one_test (impl, s2, s1, len, n); >> +} > > Ok. > >> +static void >> +do_page_tests (void) >> +{ >> + CHAR *s1, *s2; >> + const size_t maxoffset = 64; >> + >> + /* Put s1 at the maxoffset from the edge of buf1's last page. */ >> + s1 = (CHAR *) buf1 + BUF1PAGES * page_size / sizeof(CHAR) - maxoffset; >> + /* s2 needs room to put a string with size of maxoffset + 1 at s2 + >> + (maxoffset - 1). */ >> + s2 = (CHAR *) buf2 + page_size / sizeof(CHAR) - maxoffset * 2; >> + >> + MEMSET (s1, 'a', maxoffset - 1); >> + s1[maxoffset - 1] = '\0'; >> + >> + /* Both strings are bounded to a page with read/write access and the next >> + page is protected with PROT_NONE (meaning that any access outside of the >> + page regions will trigger an invalid memory access). >> + >> + The loop copies the string s1 for all possible offsets up to maxoffset >> + for both inputs with a size larger than s1 (so memory access outside the >> + expected memory regions might trigger invalid access). */ >> + >> + for (size_t off1 = 0; off1 < maxoffset; off1++) >> + { >> + for (size_t off2 = 0; off2 < maxoffset; off2++) >> + { >> + FOR_EACH_IMPL (impl, 0) >> + do_one_test (impl, s2 + off2, s1 + off1, maxoffset - off1 - 1, >> + maxoffset + (maxoffset - off2)); >> + } >> + } >> +} > > Ok. > >> +static void >> +do_random_tests (void) >> +{ >> + size_t i, j, n, align1, align2, len, size, mode; >> + CHAR *p1 = (CHAR *) (buf1 + page_size) - 1024; >> + CHAR *p2 = (CHAR *) (buf2 + page_size) - 1024; >> + size_t res; >> + >> + for (n = 0; n < ITERATIONS; n++) >> + { >> + /* For wcslcpy: align1 and align2 here mean align not in bytes, >> + but in wchar_ts, in bytes it will equal to align * (sizeof >> + (wchar_t)). */ >> + >> + mode = random (); > > Ok. > >> + if (mode & 1) >> + { >> + size = random () & 255; >> + align1 = 512 - size - (random () & 15); >> + if (mode & 2) >> + align2 = align1 - (random () & 24); >> + else >> + align2 = align1 - (random () & 31); >> + if (mode & 4) >> + { >> + j = align1; >> + align1 = align2; >> + align2 = j; >> + } >> + if (mode & 8) >> + len = size - (random () & 31); >> + else >> + len = 512; >> + if (len >= 512) >> + len = random () & 511; >> + } > > Ok. > >> + else >> + { >> + align1 = random () & 31; >> + if (mode & 2) >> + align2 = random () & 31; >> + else >> + align2 = align1 + (random () & 24); >> + len = random () & 511; >> + j = align1; >> + if (align2 > j) >> + j = align2; >> + if (mode & 4) >> + { >> + size = random () & 511; >> + if (size + j > 512) >> + size = 512 - j - (random () & 31); >> + } >> + else >> + size = 512 - j; >> + if ((mode & 8) && len + j >= 512) >> + len = 512 - j - (random () & 7); >> + } > > end of mode 1 > >> + j = len + align1; >> + for (i = 0; i < j; i++) >> + { >> + p1[i] = random () & BIG_CHAR; >> + if (i >= align1 && i < len + align1 && !p1[i]) >> + p1[i] = (random () & SMALL_CHAR) + 3; >> + } >> + p1[i] = 0; > > Ok. string is now j (len+align1) chars long. > >> + FOR_EACH_IMPL (impl, 1) >> + { >> + MEMSET (p2 - 64, '\1', 512 + 64); >> + res = CALL (impl, (CHAR *) (p2 + align2), >> + (CHAR *) (p1 + align1), size); > > p1+align1 length would be j - align1 or len, ok. > >> + if (res != len) > > Ok. > >> + { >> + error (0, 0, "Iteration %zd - wrong result in function %s (%zd, %zd) %zd != %zd", >> + n, impl->name, align1, align2, len, res); >> + ret = 1; >> + } > > Ok. > >> + for (j = 0; j < align2 + 64; ++j) >> + { >> + if (p2[j - 64] != '\1') >> + { >> + error (0, 0, "Iteration %zd - garbage before, %s (%zd, %zd, %zd)", >> + n, impl->name, align1, align2, len); >> + ret = 1; >> + break; >> + } >> + } > > Ok. Ends at p2[align2-1] > >> + j = align2 + len + 1; >> + if (size + align2 > j) >> + j = size + align2; >> + for (; j < 512; ++j) >> + { >> + if (p2[j] != '\1') >> + { >> + error (0, 0, "Iteration %zd - garbage after, %s (%zd, %zd, %zd)", >> + n, impl->name, align1, align2, len); >> + ret = 1; >> + break; >> + } >> + } > > Ok. > >> + j = len; >> + /* Check for zero size. */ >> + if (size) >> + { >> + if (size <= j) >> + j = size - 1; >> + if (MEMCMP (p1 + align1, p2 + align2, j)) >> + { >> + error (0, 0, "Iteration %zd - different strings, %s (%zd, %zd, %zd)", >> + n, impl->name, align1, align2, len); >> + ret = 1; >> + } >> + if (p2[align2 + j]) >> + { >> + error (0, 0, "Iteration %zd - garbage after size, %s (%zd, %zd, %zd)", >> + n, impl->name, align1, align2, len); >> + ret = 1; >> + break; >> + } >> + } >> + } >> + } >> +} > > Ok. > >> +int >> +test_main (void) >> +{ >> + size_t i; >> + >> + test_init (); >> + >> + printf ("%28s", ""); >> + FOR_EACH_IMPL (impl, 0) >> + printf ("\t%s", impl->name); >> + putchar ('\n'); >> + >> + for (i = 1; i < 8; ++i) >> + { >> + do_test (i, i, 16, 16, SMALL_CHAR); >> + do_test (i, i, 16, 16, BIG_CHAR); >> + do_test (i, 2 * i, 16, 16, SMALL_CHAR); >> + do_test (2 * i, i, 16, 16, BIG_CHAR); >> + do_test (8 - i, 2 * i, 1 << i, 2 << i, SMALL_CHAR); >> + do_test (2 * i, 8 - i, 2 << i, 1 << i, SMALL_CHAR); >> + do_test (8 - i, 2 * i, 1 << i, 2 << i, BIG_CHAR); >> + do_test (2 * i, 8 - i, 2 << i, 1 << i, BIG_CHAR); >> + } >> + >> + for (i = 1; i < 8; ++i) >> + { >> + do_test (0, 0, 4 << i, 8 << i, SMALL_CHAR); >> + do_test (0, 0, 16 << i, 8 << i, SMALL_CHAR); >> + do_test (8 - i, 2 * i, 4 << i, 8 << i, SMALL_CHAR); >> + do_test (8 - i, 2 * i, 16 << i, 8 << i, SMALL_CHAR); >> + } >> + >> + do_random_tests (); >> + do_page_tests (); >> + return ret; >> +} > > Ok. > >> +#include >> diff --git a/wcsmbs/Makefile b/wcsmbs/Makefile >> index 22192985e1..3fde8c76db 100644 >> --- a/wcsmbs/Makefile >> +++ b/wcsmbs/Makefile >> @@ -134,6 +134,7 @@ tests := \ >> test-wcscpy \ >> test-wcscspn \ >> test-wcsdup \ >> + test-wcslcpy \ >> test-wcslen \ >> test-wcsncat \ >> test-wcsncmp \ > > Ok. > >> diff --git a/wcsmbs/test-wcslcpy.c b/wcsmbs/test-wcslcpy.c >> new file mode 100644 >> index 0000000000..91af5ae511 >> --- /dev/null >> +++ b/wcsmbs/test-wcslcpy.c >> @@ -0,0 +1,20 @@ >> +/* Test wcslcpy functions. >> + Copyright (C) 2023 Free Software Foundation, Inc. >> + This file is part of the GNU C Library. >> + >> + The GNU C Library is free software; you can redistribute it and/or >> + modify it under the terms of the GNU Lesser General Public >> + License as published by the Free Software Foundation; either >> + version 2.1 of the License, or (at your option) any later version. >> + >> + The GNU C Library is distributed in the hope that it will be useful, >> + but WITHOUT ANY WARRANTY; without even the implied warranty of >> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU >> + Lesser General Public License for more details. >> + >> + You should have received a copy of the GNU Lesser General Public >> + License along with the GNU C Library; if not, see >> + . */ >> + >> +#define WIDE 1 >> +#include "../string/test-strlcpy.c" > > Ok. > -- Cheers, Carlos.