[PATCH] locale: Fix localedata/sort-test undefined behavior

[PATCH] locale: Fix localedata/sort-test undefined behavior

[Adhemerval Zanella]

The collate-test.c triggers UB with an signed integer overflow,
which results in an error on some architectures (powerpc32).

Checked on x86_64, i686, and powerpc.
---
 localedata/collate-test.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/localedata/collate-test.c b/localedata/collate-test.c
index 46b91ec57f..09fd5158a7 100644
--- a/localedata/collate-test.c
+++ b/localedata/collate-test.c
@@ -86,7 +86,7 @@ main (int argc, char *argv[])
   srandom (atoi (argv[1]));
   for (n = 0; n < 10 * nstrings; ++n)
     {
-      int r1, r2, r;
+      int r1, r2;
       size_t idx1 = random () % nstrings;
       size_t idx2 = random () % nstrings;
       struct lines tmp = strings[idx1];
@@ -96,9 +96,10 @@ main (int argc, char *argv[])
       /* While we are at it a first little test.  */
       r1 = strcoll (strings[idx1].key, strings[idx2].key);
       r2 = strcoll (strings[idx2].key, strings[idx1].key);
-      r = r1 * r2;
 
-      if (r > 0 || (r == 0 && r1 != 0) || (r == 0 && r2 != 0))
+      if ((r1 > 0 && r2 > 0)
+	  || ((r1 == 0 || r2 == 0) && r1 != 0)
+	  || ((r1 == 0 || r2 == 0) && r2 != 0))
 	printf ("`%s' and `%s' collate wrong: %d vs. %d\n",
 		strings[idx1].key, strings[idx2].key, r1, r2);
     }
-- 
2.32.0

Re: [PATCH] locale: Fix localedata/sort-test undefined behavior

[Andreas Schwab]

On Nov 03 2021, Adhemerval Zanella wrote:

> The collate-test.c triggers UB with an signed integer overflow,
> which results in an error on some architectures (powerpc32).
>
> Checked on x86_64, i686, and powerpc.
> ---
>  localedata/collate-test.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/localedata/collate-test.c b/localedata/collate-test.c
> index 46b91ec57f..09fd5158a7 100644
> --- a/localedata/collate-test.c
> +++ b/localedata/collate-test.c
> @@ -86,7 +86,7 @@ main (int argc, char *argv[])
>    srandom (atoi (argv[1]));
>    for (n = 0; n < 10 * nstrings; ++n)
>      {
> -      int r1, r2, r;
> +      int r1, r2;
>        size_t idx1 = random () % nstrings;
>        size_t idx2 = random () % nstrings;
>        struct lines tmp = strings[idx1];
> @@ -96,9 +96,10 @@ main (int argc, char *argv[])
>        /* While we are at it a first little test.  */
>        r1 = strcoll (strings[idx1].key, strings[idx2].key);
>        r2 = strcoll (strings[idx2].key, strings[idx1].key);
> -      r = r1 * r2;
>  
> -      if (r > 0 || (r == 0 && r1 != 0) || (r == 0 && r2 != 0))
> +      if ((r1 > 0 && r2 > 0)

That doesn't look the same.  Shouldn't that be (r1 > 0) == (r2 > 0)?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH] locale: Fix localedata/sort-test undefined behavior

[Paul Eggert]

On 11/3/21 12:50, Andreas Schwab wrote:
>> -      if (r > 0 || (r == 0 && r1 != 0) || (r == 0 && r2 != 0))
>> +      if ((r1 > 0 && r2 > 0)
> That doesn't look the same.  Shouldn't that be (r1 > 0) == (r2 > 0)?

But that would be true when r1 == 0 && r2 == 0, whereas the original 
expression (if there's no overflow) would make it false.

On 11/3/21 12:40, Adhemerval Zanella via Libc-alpha wrote:

> -      r = r1 * r2;
>  
> -      if (r > 0 || (r == 0 && r1 != 0) || (r == 0 && r2 != 0))
> +      if ((r1 > 0 && r2 > 0)
> +	  || ((r1 == 0 || r2 == 0) && r1 != 0)
> +	  || ((r1 == 0 || r2 == 0) && r2 != 0))

This is both too-complicated and (as Andreas wrote) not quite right. 
Instead, I suggest something like this:

   if (signum (r1) != - signum (r2))

where 'signum' is defined by something like this:

static int signum (int n) { return (0 < n) - (n < 0); }

This is clearer and avoids the overflow bug.