Re: [PHP-CVS] com php-src: fix int overflow preserving the old behavior: ext/standard/strnatcmp.c

On Mon, Sep 15, 2014 at 10:11 AM, Anatol Belski <[email protected]> wrote:

> Moin,
>
> On Mon, September 15, 2014 01:01, Nikita Popov wrote:
> > On Sun, Sep 14, 2014 at 4:39 PM, Anatol Belski <[email protected]> wrote:
> >
> >
> >> Commit:    29f8b21cd71bc4af1ead7b8a93cfe09338d2eff5
> >> Author:    Anatol Belski <[email protected]>         Sun, 14 Sep 2014 16:37:38
> >> +0200
> >> Parents:   eda5ba1f8fa935b8d1b8bae9d189c6afbe833287
> >> Branches:  master
> >>
> >>
> >> Link:
> >>
> http://git.php.net/?p=php-src.git;a=commitdiff;h=29f8b21cd71bc4af1ead7b8
> >> a93cfe09338d2eff5
> >>
> >> Log:
> >> fix int overflow preserving the old behavior
> >>
> >> Changed paths:
> >> M  ext/standard/strnatcmp.c
> >>
> >>
> >>
> >> Diff:
> >> diff --git a/ext/standard/strnatcmp.c b/ext/standard/strnatcmp.c index
> >> face191..7b3826b 100644 --- a/ext/standard/strnatcmp.c
> >> +++ b/ext/standard/strnatcmp.c
> >> @@ -108,8 +108,25 @@ PHPAPI int strnatcmp_ex(char const *a, size_t
> >> a_len, char const *b, size_t b_len int fractional, result; short
> leading =
> >> 1;
> >>
> >>
> >> -       if (a_len == 0 || b_len == 0)
> >> -               return a_len - b_len;
> >> +       if (a_len == 0 || b_len == 0) {
> >> +               result = 0;
> >> +
> >> +               if (a_len > b_len) {
> >> +                       if (a_len - b_len <= INT_MAX) {
> >> +                               result = (int)(a_len - b_len);
> >> +                       } else {
> >> +                               result = 1;
> >> +                       }
> >> +               } else {
> >> +                       if (b_len - a_len <= (size_t)(-INT_MIN)) {
> >> +                               result = -(int)(b_len - a_len);
> >> +                       } else {
> >> +                               result = -1;
> >> +                       }
> >> +               }
> >> +
> >> +               return result;
> >> +       }
> >>
> >>
> >> ap = a; bp = b;
> >>
> >
> > Unless I misunderstood the purpose of this code, this looks way too
> > complicated. Shouldn't this be sufficient?
> >
> > if (a_len == 0 || b_len == 0) { return a_len == b_len ? 0 : a_len > b_len
> > ? 1 : -1;
> > }
> >
> >
> > Or:
> >
> >
> > if (b_len == 0) { return a_len > 0; } else if (a_len == 0) {
> > return -1; }
> >
> >
> Yeah, your snippets are similar to what I did when I saw this overflow.
> However 1-2 tests was failing in ext/standard as they're depending on the
> exact return value. The correct and simple way is just as you say, a
> compare function should return 1,0,-1 ... nothing else.
>
> Then I just thought - making it return zend_long (to have no overflow)
> isn't possible as there are pointers for compare functions which are
> shared. The correct solution with 1,0,-1 breaks tests (so potentially some
> userspace code). Thus this mixed solution INT_MIN < ret < INT_MAX is the
> old behavior, otherwise 1,0,-1.
>
> Simpler were to solve it correctly and fix the tests, I'd be for that
> actually. Especially if nobody thinks preserving the old behavior isn't
> worth it.
>

I have no problem if the exact return value changes. Only the sign matters.
You can't derive anything useful from the exact value anyways.

Nikita