fixed crash of cases related to mbstring on Big_Endian platform(s390x)

[samding01]

This PR fixes the 75863

Note type (StateCheckNumType) is defined as "short" (ext/mbstring/oniguruma/src/regint.h),
while "mem" is int,

191 #define PLATFORM_GET_INC(val,p,type) do{\
192   val  = *(type* )p;\
193   (p) += sizeof(type);\
194 } while(0)
...
553 #define GET_STATE_CHECK_NUM_INC(num,p)  PLATFORM_GET_INC(num, p, StateCheckNumType)

which causes the issue on Big_Endian platforms.

The solution: change StateCheckNumType as int on s390x,
and ifdef for safe on other platforms.

[cmb69]

Hmm, shouldn't that be fixed upstream?

[samding01]

@cmb69 when you refer kkos/oniguruma code, did you just copy/paste?
Do not know how to fix on kkos/oniguruma side?
By the way, kkos/oniguruma just defined StateCheckNumType like:

 fgrep -R StateCheckNumType *
src/regint.h:typedef short int StateCheckNumType;
src/regexec.c:  StateCheckNumType scn;

But does not use as in the macro and pointer operations that causes the problem on big_endian platform

[cmb69]

But does not use as in the macro and pointer operations that causes the problem on big_endian platform

Indeed, see kkos/oniguruma#86.

Anyhow, it seems we need a slightly more general solution here, since the PR only caters to s390x, but
other big-endian platforms are affected as well. I wonder whether we could simply use typedef int StateCheckNumType everywhere.

Also, it is unclear whether the fix has to be applied to PHP-7.1, too.

[samding01]

@cmb69 It is fine to simply use typedef int StateCheckNumType if you think it is safe not to limit to only s390x.
Created a PR on PHP-7.1 (#3277)
Please review

[samding01]

@cmb69 , please see the reviews on #3277. What is the suggested solution?

[cmb69]

What is the suggested solution?

For master, we don't need a fix, since StateCheckNumType is unused as of Oniguruma 6.8.2.

If the issue will not be fixed for PHP-7.1 for ABI compatibility reasons, it should not be fixed for PHP-7.2 for the same reasons.

[nikic]

@cmb69 I'd say we can merge this into 7.1, because

• There is an upstream fix in master, so it's fine to apply a custom patch on lower branches.
• S390X is not a mainstream platform, so any impact will be niche. Given that oniguruma currently doesn't work there due to this issue, I'd say the question of ABI compatibility is somewhat moot.

[cmb69]

S390X is not a mainstream platform, so any impact will be niche.

If we take this PR literally, yes. However, other big endian platforms are likely affected as well, and a more general solution (PR #3277) has been rejected by Joe for being an ABI break. I do not agree with that, though.

PS: see the related https://bugs.php.net/76183.

[samding01]

Can you let me know if this PR will be merged or not?
Thanks,

[cmb69]

@sgolemon Since this PR targets PHP-7.2, what do you think?