[PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[Wilco Dijkstra]

Hi Noah,

> These implementations are incorrect. There may be a mismatch in s1/s2
> before invalid memory but no null CHAR / length boundary.

This is not true, see eg. https://en.cppreference.com/w/cpp/string/byte/strcmp.
Most string functions require that the string is correctly terminated. There
are only a few exceptions (memchr IIRC), and several generic implementations
use strlen or strnlen before the main loop.

So if your changes to the testsuite cause these functions to fail, I believe it will
create failures on other targets too (eg. AArch64 strcmp does a check for zero
and continues reading if there is no zero).

It's OK to remove these weirdly named functions (I already removed them from
the benchtests directory), however we should ensure the strings are valid to
avoid reporting errors on existing implementations.

Cheers,
Wilco

Re: [PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[Florian Weimer]

* Wilco Dijkstra via Libc-alpha:

> Hi Noah,
>
>> These implementations are incorrect. There may be a mismatch in s1/s2
>> before invalid memory but no null CHAR / length boundary.
>
> This is not true, see
> eg. https://en.cppreference.com/w/cpp/string/byte/strcmp.  Most string
> functions require that the string is correctly terminated. There are
> only a few exceptions (memchr IIRC), and several generic
> implementations use strlen or strnlen before the main loop.

I am not sure if those are bugs.  Don't we support non-array usage in
these functions as an extension?  At least for strncmp and strnlen and
their wide counterparts.  C11 is pretty clear that strncmp operates on
arrays, so this is an extension.

Thanks,
Florian

Re: [PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[Wilco Dijkstra]

Hi Florian,

>>> These implementations are incorrect. There may be a mismatch in s1/s2
>>> before invalid memory but no null CHAR / length boundary.
>>
>> This is not true, see
>> eg. https://en.cppreference.com/w/cpp/string/byte/strcmp.  Most string
>> functions require that the string is correctly terminated. There are
>> only a few exceptions (memchr IIRC), and several generic
>> implementations use strlen or strnlen before the main loop.
>
> I am not sure if those are bugs.  Don't we support non-array usage in
> these functions as an extension?  At least for strncmp and strnlen and
> their wide counterparts.  C11 is pretty clear that strncmp operates on
> arrays, so this is an extension.

My concern is about being able to read beyond a mismatch (which you have
to anyway when you process more than 1 character per iteration) rather than
reading beyond the end of a string or array.

What do you mean with non-array usage? Reading beyond the size parameter
in a strn* function if a NUL terminator has not been found yet? Or not stopping
at NUL before the size?

Wilco

Re: [PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[Florian Weimer]

* Wilco Dijkstra:

> Hi Florian,
>
>>>> These implementations are incorrect. There may be a mismatch in s1/s2
>>>> before invalid memory but no null CHAR / length boundary.
>>>
>>> This is not true, see
>>> eg. https://en.cppreference.com/w/cpp/string/byte/strcmp.  Most string
>>> functions require that the string is correctly terminated. There are
>>> only a few exceptions (memchr IIRC), and several generic
>>> implementations use strlen or strnlen before the main loop.
>>
>> I am not sure if those are bugs.  Don't we support non-array usage in
>> these functions as an extension?  At least for strncmp and strnlen and
>> their wide counterparts.  C11 is pretty clear that strncmp operates on
>> arrays, so this is an extension.
>
> My concern is about being able to read beyond a mismatch (which you have
> to anyway when you process more than 1 character per iteration) rather than
> reading beyond the end of a string or array.
>
> What do you mean with non-array usage? Reading beyond the size
> parameter in a strn* function if a NUL terminator has not been found
> yet? Or not stopping at NUL before the size?

strncmp is commonly used as a starts-with-prefix function, as in:

  strncmp (s, "prefix", 6)

This would be the second case: stopping at NUL before the number of
specified bytes are read.

Thanks,
Florian

Re: [PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[Wilco Dijkstra]

Hi Florian,

> strncmp is commonly used as a starts-with-prefix function, as in:
>
>   strncmp (s, "prefix", 6)
>
> This would be the second case: stopping at NUL before the number of
> specified bytes are read.

Indeed, such usage is both common and natural. There is only one size parameter,
so you couldn't ever use strncmp on strings you didn't know the buffer size of,
making it useless. Compilers don't optimize this to memcmp eventhough the C
standard seems to allow that, so the accepted standard is that strn* functions
must stop at the first NUL character.

Cheers,
Wilco

Re: [PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[Noah Goldstein]

On Mon, Jan 10, 2022 at 5:42 AM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
> Hi Noah,
>
> > These implementations are incorrect. There may be a mismatch in s1/s2
> > before invalid memory but no null CHAR / length boundary.
>
> This is not true, see eg. https://en.cppreference.com/w/cpp/string/byte/strcmp.
> Most string functions require that the string is correctly terminated. There
> are only a few exceptions (memchr IIRC), and several generic implementations
> use strlen or strnlen before the main loop.
>
> So if your changes to the testsuite cause these functions to fail, I believe it will
> create failures on other targets too (eg. AArch64 strcmp does a check for zero
> and continues reading if there is no zero).
>
> It's OK to remove these weirdly named functions (I already removed them from
> the benchtests directory), however we should ensure the strings are valid to
> avoid reporting errors on existing implementations.

Good catch! Had always thought the per-byte loop that stopped on mismatch
was the functionality we had to emulate.

I'll update the commit message in V3 and make sure all the added tests
don't test this edge case.
>
> Cheers,
> Wilco

Re: [PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[Noah Goldstein]

On Mon, Jan 10, 2022 at 5:42 AM Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
>
> Hi Noah,
>
> > These implementations are incorrect. There may be a mismatch in s1/s2
> > before invalid memory but no null CHAR / length boundary.
>
> This is not true, see eg. https://en.cppreference.com/w/cpp/string/byte/strcmp.
> Most string functions require that the string is correctly terminated. There
> are only a few exceptions (memchr IIRC), and several generic implementations
> use strlen or strnlen before the main loop.
>
> So if your changes to the testsuite cause these functions to fail, I believe it will
> create failures on other targets too (eg. AArch64 strcmp does a check for zero
> and continues reading if there is no zero).
>
> It's OK to remove these weirdly named functions (I already removed them from
> the benchtests directory), however we should ensure the strings are valid to
> avoid reporting errors on existing implementations.

Changed the commit message and updated tests so that a valid null terminator
is guaranteed for all strings. Thanks for pointing this out.

>
> Cheers,
> Wilco

Re: [PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[H.J. Lu]

On Sun, Jan 9, 2022 at 4:29 PM Noah Goldstein via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> These implementations are incorrect. There may be a mismatch in s1/s2
> before invalid memory but no null CHAR / length boundary.
>
> Signed-off-by: Noah Goldstein <goldstein.w.n@gmail.com>
> ---
>  string/test-strcmp.c  | 35 -----------------------------------
>  string/test-strncmp.c | 34 ----------------------------------
>  2 files changed, 69 deletions(-)
>
> diff --git a/string/test-strcmp.c b/string/test-strcmp.c
> index 3c75076fb8..97d7bf5043 100644
> --- a/string/test-strcmp.c
> +++ b/string/test-strcmp.c
> @@ -34,7 +34,6 @@
>  # define STRLEN wcslen
>  # define MEMCPY wmemcpy
>  # define SIMPLE_STRCMP simple_wcscmp
> -# define STUPID_STRCMP stupid_wcscmp
>  # define CHAR wchar_t
>  # define UCHAR wchar_t
>  # define CHARBYTES 4
> @@ -64,25 +63,6 @@ simple_wcscmp (const wchar_t *s1, const wchar_t *s2)
>    return c1 < c2 ? -1 : 1;
>  }
>
> -int
> -stupid_wcscmp (const wchar_t *s1, const wchar_t *s2)
> -{
> -  size_t ns1 = wcslen (s1) + 1;
> -  size_t ns2 = wcslen (s2) + 1;
> -  size_t n = ns1 < ns2 ? ns1 : ns2;
> -  int ret = 0;
> -
> -  wchar_t c1, c2;
> -
> -  while (n--) {
> -    c1 = *s1++;
> -    c2 = *s2++;
> -    if ((ret = c1 < c2 ? -1 : c1 == c2 ? 0 : 1) != 0)
> -      break;
> -  }
> -  return ret;
> -}
> -
>  #else
>  # include <limits.h>
>
> @@ -92,7 +72,6 @@ stupid_wcscmp (const wchar_t *s1, const wchar_t *s2)
>  # define STRLEN strlen
>  # define MEMCPY memcpy
>  # define SIMPLE_STRCMP simple_strcmp
> -# define STUPID_STRCMP stupid_strcmp
>  # define CHAR char
>  # define UCHAR unsigned char
>  # define CHARBYTES 1
> @@ -113,24 +92,10 @@ simple_strcmp (const char *s1, const char *s2)
>    return ret;
>  }
>
> -int
> -stupid_strcmp (const char *s1, const char *s2)
> -{
> -  size_t ns1 = strlen (s1) + 1;
> -  size_t ns2 = strlen (s2) + 1;
> -  size_t n = ns1 < ns2 ? ns1 : ns2;
> -  int ret = 0;
> -
> -  while (n--)
> -    if ((ret = *(unsigned char *) s1++ - *(unsigned char *) s2++) != 0)
> -      break;
> -  return ret;
> -}
>  #endif
>
>  typedef int (*proto_t) (const CHAR *, const CHAR *);
>
> -IMPL (STUPID_STRCMP, 1)
>  IMPL (SIMPLE_STRCMP, 1)
>  IMPL (STRCMP, 1)
>
> diff --git a/string/test-strncmp.c b/string/test-strncmp.c
> index e7d5edea39..61a283a0af 100644
> --- a/string/test-strncmp.c
> +++ b/string/test-strncmp.c
> @@ -33,7 +33,6 @@
>  # define STRDUP wcsdup
>  # define MEMCPY wmemcpy
>  # define SIMPLE_STRNCMP simple_wcsncmp
> -# define STUPID_STRNCMP stupid_wcsncmp
>  # define CHAR wchar_t
>  # define UCHAR wchar_t
>  # define CHARBYTES 4
> @@ -57,25 +56,6 @@ simple_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
>    return 0;
>  }
>
> -int
> -stupid_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
> -{
> -  wchar_t c1, c2;
> -  size_t ns1 = wcsnlen (s1, n) + 1, ns2 = wcsnlen (s2, n) + 1;
> -
> -  n = ns1 < n ? ns1 : n;
> -  n = ns2 < n ? ns2 : n;
> -
> -  while (n--)
> -    {
> -      c1 = *s1++;
> -      c2 = *s2++;
> -      if (c1 != c2)
> -       return c1 > c2 ? 1 : -1;
> -    }
> -  return 0;
> -}
> -
>  #else
>  # define L(str) str
>  # define STRNCMP strncmp
> @@ -83,7 +63,6 @@ stupid_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
>  # define STRDUP strdup
>  # define MEMCPY memcpy
>  # define SIMPLE_STRNCMP simple_strncmp
> -# define STUPID_STRNCMP stupid_strncmp
>  # define CHAR char
>  # define UCHAR unsigned char
>  # define CHARBYTES 1
> @@ -101,23 +80,10 @@ simple_strncmp (const char *s1, const char *s2, size_t n)
>    return ret;
>  }
>
> -int
> -stupid_strncmp (const char *s1, const char *s2, size_t n)
> -{
> -  size_t ns1 = strnlen (s1, n) + 1, ns2 = strnlen (s2, n) + 1;
> -  int ret = 0;
> -
> -  n = ns1 < n ? ns1 : n;
> -  n = ns2 < n ? ns2 : n;
> -  while (n-- && (ret = *(unsigned char *) s1++ - * (unsigned char *) s2++) == 0);
> -  return ret;
> -}
> -
>  #endif
>
>  typedef int (*proto_t) (const CHAR *, const CHAR *, size_t);
>
> -IMPL (STUPID_STRNCMP, 0)
>  IMPL (SIMPLE_STRNCMP, 0)
>  IMPL (STRNCMP, 1)
>
> --
> 2.25.1
>

LGTM.

Reviewed-by: H.J. Lu <hjl.tools@gmail.com>

Thanks.

-- 
H.J.

[PATCH v2 3/7] string/test-str*cmp: remove stupid_[strcmp, strncmp, wcscmp, wcsncmp].

[Noah Goldstein]

These implementations are incorrect. There may be a mismatch in s1/s2
before invalid memory but no null CHAR / length boundary.

Signed-off-by: Noah Goldstein <goldstein.w.n@gmail.com>
---
 string/test-strcmp.c  | 35 -----------------------------------
 string/test-strncmp.c | 34 ----------------------------------
 2 files changed, 69 deletions(-)

diff --git a/string/test-strcmp.c b/string/test-strcmp.c
index 3c75076fb8..97d7bf5043 100644
--- a/string/test-strcmp.c
+++ b/string/test-strcmp.c
@@ -34,7 +34,6 @@
 # define STRLEN wcslen
 # define MEMCPY wmemcpy
 # define SIMPLE_STRCMP simple_wcscmp
-# define STUPID_STRCMP stupid_wcscmp
 # define CHAR wchar_t
 # define UCHAR wchar_t
 # define CHARBYTES 4
@@ -64,25 +63,6 @@ simple_wcscmp (const wchar_t *s1, const wchar_t *s2)
   return c1 < c2 ? -1 : 1;
 }
 
-int
-stupid_wcscmp (const wchar_t *s1, const wchar_t *s2)
-{
-  size_t ns1 = wcslen (s1) + 1;
-  size_t ns2 = wcslen (s2) + 1;
-  size_t n = ns1 < ns2 ? ns1 : ns2;
-  int ret = 0;
-
-  wchar_t c1, c2;
-
-  while (n--) {
-    c1 = *s1++;
-    c2 = *s2++;
-    if ((ret = c1 < c2 ? -1 : c1 == c2 ? 0 : 1) != 0)
-      break;
-  }
-  return ret;
-}
-
 #else
 # include <limits.h>
 
@@ -92,7 +72,6 @@ stupid_wcscmp (const wchar_t *s1, const wchar_t *s2)
 # define STRLEN strlen
 # define MEMCPY memcpy
 # define SIMPLE_STRCMP simple_strcmp
-# define STUPID_STRCMP stupid_strcmp
 # define CHAR char
 # define UCHAR unsigned char
 # define CHARBYTES 1
@@ -113,24 +92,10 @@ simple_strcmp (const char *s1, const char *s2)
   return ret;
 }
 
-int
-stupid_strcmp (const char *s1, const char *s2)
-{
-  size_t ns1 = strlen (s1) + 1;
-  size_t ns2 = strlen (s2) + 1;
-  size_t n = ns1 < ns2 ? ns1 : ns2;
-  int ret = 0;
-
-  while (n--)
-    if ((ret = *(unsigned char *) s1++ - *(unsigned char *) s2++) != 0)
-      break;
-  return ret;
-}
 #endif
 
 typedef int (*proto_t) (const CHAR *, const CHAR *);
 
-IMPL (STUPID_STRCMP, 1)
 IMPL (SIMPLE_STRCMP, 1)
 IMPL (STRCMP, 1)
 
diff --git a/string/test-strncmp.c b/string/test-strncmp.c
index e7d5edea39..61a283a0af 100644
--- a/string/test-strncmp.c
+++ b/string/test-strncmp.c
@@ -33,7 +33,6 @@
 # define STRDUP wcsdup
 # define MEMCPY wmemcpy
 # define SIMPLE_STRNCMP simple_wcsncmp
-# define STUPID_STRNCMP stupid_wcsncmp
 # define CHAR wchar_t
 # define UCHAR wchar_t
 # define CHARBYTES 4
@@ -57,25 +56,6 @@ simple_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
   return 0;
 }
 
-int
-stupid_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
-{
-  wchar_t c1, c2;
-  size_t ns1 = wcsnlen (s1, n) + 1, ns2 = wcsnlen (s2, n) + 1;
-
-  n = ns1 < n ? ns1 : n;
-  n = ns2 < n ? ns2 : n;
-
-  while (n--)
-    {
-      c1 = *s1++;
-      c2 = *s2++;
-      if (c1 != c2)
-	return c1 > c2 ? 1 : -1;
-    }
-  return 0;
-}
-
 #else
 # define L(str) str
 # define STRNCMP strncmp
@@ -83,7 +63,6 @@ stupid_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
 # define STRDUP strdup
 # define MEMCPY memcpy
 # define SIMPLE_STRNCMP simple_strncmp
-# define STUPID_STRNCMP stupid_strncmp
 # define CHAR char
 # define UCHAR unsigned char
 # define CHARBYTES 1
@@ -101,23 +80,10 @@ simple_strncmp (const char *s1, const char *s2, size_t n)
   return ret;
 }
 
-int
-stupid_strncmp (const char *s1, const char *s2, size_t n)
-{
-  size_t ns1 = strnlen (s1, n) + 1, ns2 = strnlen (s2, n) + 1;
-  int ret = 0;
-
-  n = ns1 < n ? ns1 : n;
-  n = ns2 < n ? ns2 : n;
-  while (n-- && (ret = *(unsigned char *) s1++ - * (unsigned char *) s2++) == 0);
-  return ret;
-}
-
 #endif
 
 typedef int (*proto_t) (const CHAR *, const CHAR *, size_t);
 
-IMPL (STUPID_STRNCMP, 0)
 IMPL (SIMPLE_STRNCMP, 0)
 IMPL (STRNCMP, 1)
 
-- 
2.25.1