Re: 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 <ab@php.net> wrote: > Moin, > > On Mon, September 15, 2014 01:01, Nikita Popov wrote: > > On Sun, Sep 14, 2014 at 4:39 PM, Anatol Belski <ab@php.net> wrote: > > > > > >> Commit: 29f8b21cd71bc4af1ead7b8a93cfe09338d2eff5 > >> Author: Anatol Belski <ab@php.net> 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