Use correct signedness in wcsncmp.

Message ID mg0rcq$13o$1@ger.gmane.org
State Committed
Headers

Commit Message

Stefan Liebler April 7, 2015, 2:59 p.m. UTC
  On 04/07/2015 11:55 AM, Andreas Schwab wrote:
> Stefan Liebler <stli@linux.vnet.ibm.com> writes:
>
>> @@ -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);
>
> Please add a space before '*' and after the cast.
>
>>     if (align1 < align_n)
>> -    s1 -= (align_n - align1);
>> +    s1 = (CHAR*) (((char*)s1) - (align_n - align1));
>
> Please remove the redundant parens around the cast expression.
>
> Andreas.
>
I have changed the patch.
Bye.

---
2015-04-07  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):
	  Do not 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

Andreas Schwab April 7, 2015, 4:10 p.m. UTC | #1
Stefan Liebler <stli@linux.vnet.ibm.com> writes:

> 2015-04-07  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):
> 	  Do not 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.

Ok, thanks.

Andreas.
  
Andreas Krebbel April 13, 2015, 7:28 p.m. UTC | #2
On Tue, Apr 07, 2015 at 04:59:38PM +0200, Stefan Liebler wrote:
> 2015-04-07  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):
> 	  Do not 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.

Applied. Thanks!

-Andreas-
  

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..fb57a9b 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;
 }