Fix out-of bounds access

[Zenju]

Test case: strnatcmp_ex(L"333", 3, L"333 ", 4, true)
The reason this bug didn't come up earlier is probably because most input strings are null-terminated.

[staabm]

Should be covered by a test

[Zenju]

The "isspace" calls also miss bounds-checks, except for input that is null-terminated.

while (isspace((int)(unsigned char)ca)) { ca = *++ap; }

also the following "isdigit" should check for bounds.

[Zenju]

Also the behavior of "compare_left" is strange. The result is unlike what other "natural" string comparison functions would do,e .g.. CompareString + SORT_DIGITSASNUMBERS on Windows, or CFStringCompare + kCFCompareNumerically on macOS. Maybe the whole function should be rewritten?

[laruence]

could you please add a test to trigger this bug? (as long as the problem could be seen via valgrind)

[nikic]

It's probably not possible to test this from PHP, as this assumes non-terminated strings.

I've ran into this issue recently when fixing issues with parse_url on non-terminated strings. Maybe we can introduce something like ext/test where we can include tests like this?

[krakjoe]

@nikic that sounds reasonable to me, may be useful for many other things also ...

[nikic]

Test extension has been added in #2414, it is now possible to write a test for this. Example of how an unterminated string test looks like: https://github.com/nikic/php-src/blob/e81452b5c39a950272d3606a07c3591f70414960/ext/standard/tests/url/parse_url_unterminated.phpt

[krakjoe]

@Zenju please update this PR with a test ...

[cmb69]

Regression test:

--TEST--
strnat(case)cmp(): potential OOB access for unterminated strings
--SKIPIF--
<?php
if (!function_exists('zend_create_unterminated_string')) die('skip zend_test extension not available');
?>
--FILE--
<?php
$a = zend_create_unterminated_string('333');
$b = zend_create_unterminated_string('333 ');
var_dump(
    strnatcmp($a, $b),
    strnatcasecmp($b, $a)
);
zend_terminate_string($a);
zend_terminate_string($b);
?>
===DONE===
--EXPECT--
int(-1)
int(1)
===DONE===

[weltling]

Pushed both the @Zenju's patch and the Christoph's test. Valgrind is happy.

@Zenju if you'd like to contribute more, please pay attention to the testability. Bare patches suffice in some cases, but usually a test is required.

Thanks.