Use correct signedness in wcsncmp.

Message ID mf0dja$acn$1@ger.gmane.org
State Superseded
Headers

Commit Message

Stefan Liebler March 26, 2015, 7:47 a.m. UTC
  Hi,

the wcsncmp implementation does not behave the same as wcscmp
and wmemcmp due to using substraction instead of comparision:

wchar_t w0[] = { WCHAR_MIN, 0 };
wchar_t w1[] = { WCHAR_MAX, 0 };

wcscmp (w0, w1) = -1
wcsncmp (w0, w1, 2) = 1
wmemcmp (w0, w1, 2) = -1


This patch adjusts wcsncmp behavior in the same way,
as it was done for wcscmp and wmemcmp:
-"Use correct signedness in default implementations of wcscmp and wmemcmp"
(https://sourceware.org/git/?p=glibc.git;a=commit;h=1f1e194720177de3cbc2a84bc62d22e63cb23d4a)

-"Fix signedness in wcscmp comparison"
(https://sourceware.org/git/?p=glibc.git;a=commit;h=95584d3b3309ff4da4cc458254df383f5cff044b)

The testcase localedata/tests-mbwc/tst_wcsncmp.c expects the exact
character difference, therefore it is adjusted in the same way as
"wcscmp testsuite too strict"
(https://sourceware.org/ml/libc-alpha/2000-10/msg00001.html).

The testcase string/test-strncmp.c now supports testing wcsncmp if WIDE
is defined. The corresponding testcase wcsmbs/test-wcsncmp.c is added.

Tested on s390x and x86_64.

Ok to commit?

Bye
Stefan

---
2015-03-26  Stefan Liebler  <stli@linux.vnet.ibm.com>

	* wcsmbs/wcsncmp.c (wcsncmp): Compare as wchar_t, not wint_t.
	  Use signed comparision instead of substraction to avoid
	  overflow bug.
	* localedata/tests-mbwc/tst_wcsncmp.c (tst_wcsncmp):
	  Take the sign of ret.
	* localedata/tests-mbwc/dat_wcsncmp.c (tst_wcsncmp_loc):
	  DonĀ“t expect precise return values. Only the sign matters.
	* wcsmbs/Makefile (strop-tests): Add wcsncmp.
	* wcsmbs/test-wcsncmp.c: New File.
	* string/test-strncmp.c: Add wcsncmp support.
  

Comments

Joseph Myers April 2, 2015, 5:36 p.m. UTC | #1
Patches for bugs user-visible in a release should have a bug filed in 
Bugzilla if not already there, and [BZ #N] in the ChangeLog entry.
  

Patch

diff --git a/localedata/tests-mbwc/dat_wcsncmp.c b/localedata/tests-mbwc/dat_wcsncmp.c
index 167ce48..f468a8b 100644
--- a/localedata/tests-mbwc/dat_wcsncmp.c
+++ b/localedata/tests-mbwc/dat_wcsncmp.c
@@ -33,7 +33,7 @@  TST_WCSNCMP tst_wcsncmp_loc [] = {
       },
       { /*input.*/ { { 0x00D1,0x00D2,0x00D3,0x0000 },
 		     { 0x0000,0x00D2,0x00D3,0x0000 }, 3 },  /* #06 */
-	/*expect*/ { 0,1,0x00D1,			},
+	/*expect*/ { 0,1,1,			},
       },
       { /*input.*/ { { 0x00D1,0x00D2,0x00D3,0x0000 },
 		     { 0x00D1,0x00D2,0x00D9,0x0000 }, 2 },  /* #07 */
@@ -41,11 +41,11 @@  TST_WCSNCMP tst_wcsncmp_loc [] = {
       },
       { /*input.*/ { { 0x00D1,0x00D2,0x00D3,0x0000 },
 		     { 0x00D1,0x00D2,0x00D9,0x0000 }, 3 },  /* #08 */
-	/*expect*/ { 0,1,-0x0006,			},
+	/*expect*/ { 0,1,-1,			},
       },
       { /*input.*/ { { 0x00D1,0x00D2,0x00D3,0x0000 },
 		     { 0x00D1,0x00D2,0x0000	   }, 4 },  /* #09 */
-	/*expect*/ { 0,1,0x00D3,			},
+	/*expect*/ { 0,1,1,			},
       },
       { .is_last = 1 }
     }
@@ -75,7 +75,7 @@  TST_WCSNCMP tst_wcsncmp_loc [] = {
       },
       { /*input.*/ { { 0x0041,0x0042,0x0043,0x0000 },
 		     { 0x0000,0x0042,0x0043,0x0000 }, 3 },  /* #06 */
-	/*expect*/ { 0,1,0x0041,			},
+	/*expect*/ { 0,1,1,			},
       },
       { /*input.*/ { { 0x0041,0x0042,0x0043,0x0000 },
 		     { 0x0041,0x0042,0x0049,0x0000 }, 2 },  /* #07 */
@@ -83,11 +83,11 @@  TST_WCSNCMP tst_wcsncmp_loc [] = {
       },
       { /*input.*/ { { 0x0041,0x0042,0x0043,0x0000 },
 		     { 0x0041,0x0042,0x0049,0x0000 }, 3 },  /* #08 */
-	/*expect*/ { 0,1,-0x0006,			},
+	/*expect*/ { 0,1,-1,			},
       },
       { /*input.*/ { { 0x0041,0x0042,0x0043,0x0000 },
 		     { 0x0041,0x0042,0x0000	   }, 4 },  /* #09 */
-	/*expect*/ { 0,1,0x0043,			},
+	/*expect*/ { 0,1,1,			},
       },
       { .is_last = 1 }
     }
@@ -117,7 +117,7 @@  TST_WCSNCMP tst_wcsncmp_loc [] = {
       },
       { /*input.*/ { { 0x3041,0x3042,0x3043,0x0000 },
 		     { 0x0000,0x3042,0x3043,0x0000 }, 3 },  /* #06 */
-	/*expect*/ { 0,1,0x3041,			},
+	/*expect*/ { 0,1,1,			},
       },
       { /*input.*/ { { 0x3041,0x3042,0x3043,0x0000 },
 		     { 0x3041,0x3042,0x3049,0x0000 }, 2 },  /* #07 */
@@ -125,11 +125,11 @@  TST_WCSNCMP tst_wcsncmp_loc [] = {
       },
       { /*input.*/ { { 0x3041,0x3042,0x3043,0x0000 },
 		     { 0x3041,0x3042,0x3049,0x0000 }, 3 },  /* #08 */
-	/*expect*/ { 0,1,-0x0006,			},
+	/*expect*/ { 0,1,-1,			},
       },
       { /*input.*/ { { 0x3041,0x3042,0x3043,0x0000 },
 		     { 0x3041,0x3042,0x0000	   }, 4 },  /* #09 */
-	/*expect*/ { 0,1,0x3043,			},
+	/*expect*/ { 0,1,1,			},
       },
       { .is_last = 1 }
     }
diff --git a/localedata/tests-mbwc/tst_wcsncmp.c b/localedata/tests-mbwc/tst_wcsncmp.c
index d046ecd..f93ca49 100644
--- a/localedata/tests-mbwc/tst_wcsncmp.c
+++ b/localedata/tests-mbwc/tst_wcsncmp.c
@@ -24,6 +24,7 @@  tst_wcsncmp (FILE * fp, int debug_flg)
       ws2 = TST_INPUT (wcsncmp).ws2;
       n = TST_INPUT (wcsncmp).n;
       ret = wcsncmp (ws1, ws2, n);
+      ret = (ret > 0 ? 1 : ret < 0 ? -1 : 0);
 
       if (debug_flg)
 	{
diff --git a/string/test-strncmp.c b/string/test-strncmp.c
index aaf4b4a..5774e31 100644
--- a/string/test-strncmp.c
+++ b/string/test-strncmp.c
@@ -1,4 +1,4 @@ 
-/* Test and measure strncmp functions.
+/* Test strncmp and wcsncmp functions.
    Copyright (C) 1999-2015 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
    Written by Jakub Jelinek <jakub@redhat.com>, 1999.
@@ -18,17 +18,80 @@ 
    <http://www.gnu.org/licenses/>.  */
 
 #define TEST_MAIN
-#define TEST_NAME "strncmp"
+#ifdef WIDE
+# define TEST_NAME "wcsncmp"
+#else
+# define TEST_NAME "strncmp"
+#endif
 #include "test-string.h"
 
-typedef int (*proto_t) (const char *, const char *, size_t);
-int simple_strncmp (const char *, const char *, size_t);
-int stupid_strncmp (const char *, const char *, size_t);
+#ifdef WIDE
+# include <wchar.h>
+
+# define L(str) L##str
+# define STRNCMP wcsncmp
+# define STRCPY wcscpy
+# 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
+# define CHAR__MAX WCHAR_MAX
+# define CHAR__MIN WCHAR_MIN
+
+/* Wcsncmp uses signed semantics for comparison, not unsigned.
+   Avoid using substraction since possible overflow */
+int
+simple_wcsncmp (const CHAR *s1, const CHAR *s2, size_t n)
+{
+  wchar_t c1, c2;
+
+  while (n--)
+    {
+      c1 = *s1++;
+      c2 = *s2++;
+      if (c1 == L('\0') || c1 != c2)
+	return c1 > c2 ? 1 : (c1 < c2 ? -1 : 0);
+    }
+  return 0;
+}
 
-IMPL (stupid_strncmp, 0)
-IMPL (simple_strncmp, 0)
-IMPL (strncmp, 1)
+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
+# define STRCPY strcpy
+# 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
+# define CHAR__MAX CHAR_MAX
+# define CHAR__MIN CHAR_MIN
+
+/* Strncmp uses unsigned semantics for comparison. */
 int
 simple_strncmp (const char *s1, const char *s2, size_t n)
 {
@@ -51,8 +114,17 @@  stupid_strncmp (const char *s1, const char *s2, size_t n)
   return ret;
 }
 
+#endif
+
+typedef int (*proto_t) (const CHAR *, const CHAR *, size_t);
+
+IMPL (STUPID_STRNCMP, 0)
+IMPL (SIMPLE_STRNCMP, 0)
+IMPL (STRNCMP, 1)
+
+
 static int
-check_result (impl_t *impl, const char *s1, const char *s2, size_t n,
+check_result (impl_t *impl, const CHAR *s1, const CHAR *s2, size_t n,
 	     int exp_result)
 {
   int result = CALL (impl, s1, s2, n);
@@ -70,7 +142,7 @@  check_result (impl_t *impl, const char *s1, const char *s2, size_t n,
 }
 
 static void
-do_one_test (impl_t *impl, const char *s1, const char *s2, size_t n,
+do_one_test (impl_t *impl, const CHAR *s1, const CHAR *s2, size_t n,
 	     int exp_result)
 {
   if (check_result (impl, s1, s2, n, exp_result) < 0)
@@ -82,12 +154,12 @@  do_test_limit (size_t align1, size_t align2, size_t len, size_t n, int max_char,
 	 int exp_result)
 {
   size_t i, align_n;
-  char *s1, *s2;
+  CHAR *s1, *s2;
 
   if (n == 0)
     {
-      s1 = (char*)(buf1 + page_size);
-      s2 = (char*)(buf2 + page_size);
+      s1 = (CHAR*)(buf1 + page_size);
+      s2 = (CHAR*)(buf2 + page_size);
 
       FOR_EACH_IMPL (impl, 0)
 	do_one_test (impl, s1, s2, n, 0);
@@ -97,16 +169,16 @@  do_test_limit (size_t align1, size_t align2, size_t len, size_t n, int max_char,
 
   align1 &= 15;
   align2 &= 15;
-  align_n = (page_size - n) & 15;
+  align_n = (page_size - n * CHARBYTES) & 15;
 
-  s1 = (char*)(buf1 + page_size - n);
-  s2 = (char*)(buf2 + page_size - n);
+  s1 = (CHAR*)(buf1 + page_size - n * CHARBYTES);
+  s2 = (CHAR*)(buf2 + page_size - n * CHARBYTES);
 
   if (align1 < align_n)
-    s1 -= (align_n - align1);
+    s1 = (CHAR*) (((char*)s1) - (align_n - align1));
 
   if (align2 < align_n)
-    s2 -= (align_n - align2);
+    s2 = (CHAR*) (((char*)s2) - (align_n - align2));
 
   for (i = 0; i < n; i++)
     s1[i] = s2[i] = 1 + 23 * i % max_char;
@@ -130,24 +202,24 @@  do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char,
 	 int exp_result)
 {
   size_t i;
-  char *s1, *s2;
+  CHAR *s1, *s2;
 
   if (n == 0)
     return;
 
-  align1 &= 7;
-  if (align1 + n + 1 >= page_size)
+  align1 &= 63;
+  if (align1 + (n + 1) * CHARBYTES >= page_size)
     return;
 
-  align2 &= 7;
-  if (align2 + n + 1 >= page_size)
+  align2 &= 63;
+  if (align2 + (n + 1) * CHARBYTES >= page_size)
     return;
 
-  s1 = (char*)(buf1 + align1);
-  s2 = (char*)(buf2 + align2);
+  s1 = (CHAR*)(buf1 + align1);
+  s2 = (CHAR*)(buf2 + align2);
 
   for (i = 0; i < n; i++)
-    s1[i] = s2[i] = 1 + 23 * i % max_char;
+    s1[i] = s2[i] = 1 + (23 << ((CHARBYTES - 1) * 8)) * i % max_char;
 
   s1[n] = 24 + exp_result;
   s2[n] = 23;
@@ -161,19 +233,20 @@  do_test (size_t align1, size_t align2, size_t len, size_t n, int max_char,
     s2[n - 1] -= exp_result;
 
   FOR_EACH_IMPL (impl, 0)
-    do_one_test (impl, (char*)s1, (char*)s2, n, exp_result);
+    do_one_test (impl, s1, s2, n, exp_result);
 }
 
 static void
-do_page_test (size_t offset1, size_t offset2, char *s2)
+do_page_test (size_t offset1, size_t offset2, CHAR *s2)
 {
-  char *s1;
+  CHAR *s1;
   int exp_result;
 
-  if (offset1 >= page_size || offset2 >= page_size)
+  if (offset1 * CHARBYTES  >= page_size || offset2 * CHARBYTES >= page_size)
     return;
 
-  s1 = (char *) (buf1 + offset1);
+  s1 = (CHAR *) buf1;
+  s1 += offset1;
   s2 += offset2;
 
   exp_result= *s1;
@@ -191,8 +264,8 @@  do_random_tests (void)
   size_t i, j, n, align1, align2, pos, len1, len2, size;
   int result;
   long r;
-  unsigned char *p1 = buf1 + page_size - 512;
-  unsigned char *p2 = buf2 + page_size - 512;
+  UCHAR *p1 = (UCHAR *) (buf1 + page_size - 512 * CHARBYTES);
+  UCHAR *p2 = (UCHAR *) (buf2 + page_size - 512 * CHARBYTES);
 
   for (n = 0; n < ITERATIONS; n++)
     {
@@ -240,7 +313,7 @@  do_random_tests (void)
 	}
 
       result = 0;
-      memcpy (p2 + align2, p1 + align1, pos);
+      MEMCPY (p2 + align2, p1 + align1, pos);
       if (pos < len1)
 	{
 	  if (p2[align2 + pos] == p1[align1 + pos])
@@ -263,7 +336,7 @@  do_random_tests (void)
 
       FOR_EACH_IMPL (impl, 1)
 	{
-	  r = CALL (impl, (char*)(p1 + align1), (char*)(p2 + align2), size);
+	  r = CALL (impl, (CHAR*)(p1 + align1), (CHAR*)(p2 + align2), size);
 	  /* Test whether on 64-bit architectures where ABI requires
 	     callee to promote has the promotion been done.  */
 	  asm ("" : "=g" (r) : "0" (r));
@@ -282,19 +355,26 @@  do_random_tests (void)
 static void
 check1 (void)
 {
-  char *s1 = (char *)(buf1 + 0xb2c);
-  char *s2 = (char *)(buf1 + 0xfd8);
-  size_t i;
+  CHAR *s1 = (CHAR *)(buf1 + 0xb2c);
+  CHAR *s2 = (CHAR *)(buf1 + 0xfd8);
+  size_t i, offset;
   int exp_result;
 
-  strcpy(s1, "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrs");
-  strcpy(s2, "abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijkLMNOPQRSTUV");
+  STRCPY(s1, L("abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrs"));
+  STRCPY(s2, L("abcdefghijklmnopqrstuvwxyzabcdefghijklmnopqrstuvwxyzabcdefghijkLMNOPQRSTUV"));
+
+  /* Check possible overflow bug for wcsncmp */
+  s1[4] = CHAR__MAX;
+  s2[4] = CHAR__MIN;
 
-  for (i = 0; i < 80; i++)
+  for (offset = 0; offset < 6; offset++)
     {
-      exp_result = simple_strncmp (s1, s2, i);
-      FOR_EACH_IMPL (impl, 0)
-	 check_result (impl, s1, s2, i, exp_result);
+      for (i = 0; i < 80; i++)
+	{
+	  exp_result = SIMPLE_STRNCMP (s1 + offset, s2 + offset, i);
+	  FOR_EACH_IMPL (impl, 0)
+	    check_result (impl, s1 + offset, s2 + offset, i, exp_result);
+	}
     }
 }
 
@@ -302,17 +382,17 @@  static void
 check2 (void)
 {
   size_t i;
-  char *s1, *s2;
+  CHAR *s1, *s2;
 
-  s1 = (char *) buf1;
-  for (i = 0; i < page_size - 1; i++)
+  s1 = (CHAR *) buf1;
+  for (i = 0; i < (page_size / CHARBYTES) - 1; i++)
     s1[i] = 23;
   s1[i] = 0;
 
-  s2 = strdup (s1);
+  s2 = STRDUP (s1);
 
   for (i = 0; i < 64; ++i)
-    do_page_test (3990 + i, 2635, s2);
+    do_page_test ((3988 / CHARBYTES) + i, (2636 / CHARBYTES), s2);
 
   free (s2);
 }
diff --git a/wcsmbs/Makefile b/wcsmbs/Makefile
index 69f7892..44a4494 100644
--- a/wcsmbs/Makefile
+++ b/wcsmbs/Makefile
@@ -42,7 +42,7 @@  routines := wcscat wcschr wcscmp wcscpy wcscspn wcsdup wcslen wcsncat \
 	    isoc99_swscanf isoc99_vswscanf \
 	    mbrtoc16 c16rtomb
 
-strop-tests :=  wcscmp wmemcmp wcslen wcschr wcsrchr wcscpy
+strop-tests :=  wcscmp wcsncmp wmemcmp wcslen wcschr wcsrchr wcscpy
 tests := tst-wcstof wcsmbs-tst1 tst-wcsnlen tst-btowc tst-mbrtowc \
 	 tst-wcrtomb tst-wcpncpy tst-mbsrtowcs tst-wchar-h tst-mbrtowc2 \
 	 tst-c16c32-1 wcsatcliff $(addprefix test-,$(strop-tests))
diff --git a/wcsmbs/test-wcsncmp.c b/wcsmbs/test-wcsncmp.c
new file mode 100644
index 0000000..07757d8
--- /dev/null
+++ b/wcsmbs/test-wcsncmp.c
@@ -0,0 +1,2 @@ 
+#define WIDE 1
+#include "../string/test-strncmp.c"
diff --git a/wcsmbs/wcsncmp.c b/wcsmbs/wcsncmp.c
index 59e003b..e083ad8 100644
--- a/wcsmbs/wcsncmp.c
+++ b/wcsmbs/wcsncmp.c
@@ -29,42 +29,42 @@  wcsncmp (s1, s2, n)
      const wchar_t *s2;
      size_t n;
 {
-  wint_t c1 = L'\0';
-  wint_t c2 = L'\0';
+  wchar_t c1 = L'\0';
+  wchar_t c2 = L'\0';
 
   if (n >= 4)
     {
       size_t n4 = n >> 2;
       do
 	{
-	  c1 = (wint_t) *s1++;
-	  c2 = (wint_t) *s2++;
+	  c1 = *s1++;
+	  c2 = *s2++;
 	  if (c1 == L'\0' || c1 != c2)
-	    return c1 - c2;
-	  c1 = (wint_t) *s1++;
-	  c2 = (wint_t) *s2++;
+	    return c1 > c2 ? 1 : (c1 < c2 ? -1 : 0);
+	  c1 = *s1++;
+	  c2 = *s2++;
 	  if (c1 == L'\0' || c1 != c2)
-	    return c1 - c2;
-	  c1 = (wint_t) *s1++;
-	  c2 = (wint_t) *s2++;
+	    return c1 > c2 ? 1 : (c1 < c2 ? -1 : 0);
+	  c1 = *s1++;
+	  c2 = *s2++;
 	  if (c1 == L'\0' || c1 != c2)
-	    return c1 - c2;
-	  c1 = (wint_t) *s1++;
-	  c2 = (wint_t) *s2++;
+	    return c1 > c2 ? 1 : (c1 < c2 ? -1 : 0);
+	  c1 = *s1++;
+	  c2 = *s2++;
 	  if (c1 == L'\0' || c1 != c2)
-	    return c1 - c2;
+	    return c1 > c2 ? 1 : (c1 < c2 ? -1 : 0);
 	} while (--n4 > 0);
       n &= 3;
     }
 
   while (n > 0)
     {
-      c1 = (wint_t) *s1++;
-      c2 = (wint_t) *s2++;
+      c1 = *s1++;
+      c2 = *s2++;
       if (c1 == L'\0' || c1 != c2)
-	return c1 - c2;
+	return c1 > c2 ? 1 : (c1 < c2 ? -1 : 0);
       n--;
     }
 
-  return c1 - c2;
+  return 0;
 }