Message ID | 20211103194051.1541716-1-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | locale: Fix localedata/sort-test undefined behavior | expand |
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.
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.
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); }