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

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. Regards Anatol