Fix mbstring func overload

[Minishlink]

PHPASN1 is a dependancy of web-push-php. Some of my users have mbstring.func_overload set and thus the library is (in part) broken because of some uses of strlen and substr in PHPASN1.

This PR replaces strlen and substr occurences with their unicode safe counterpart (mb_strlen and mb_substr).

Cf. web-push-libs/web-push-php#79

[coveralls]

Coverage remained the same at 78.243% when pulling 7f6645b on Minishlink:fix_mbstring_overload into a18b162 on fgrosse:master.

[fgrosse]

Hey @Minishlink thanks for your contribution. I am generally ok with this but you should add the requirement on the extension to the composer.json and README.md.

[Minishlink]

Hi @fgrosse, sure, will do!

[coveralls]

Coverage remained the same at 78.243% when pulling 45fc4ff on Minishlink:fix_mbstring_overload into a18b162 on fgrosse:master.

[fgrosse]

After giving this PR more thought I'm now a bit hesitant. PHPASN1 does not require mbstring by itself and yet this would become a dependency of the library. IMHO using mbstring.func_overload isn't such a great idea (especially if your code deals with binary encodings) but I guess this is not helping your users.

Maybe we can instead opt-in by detecting if the mbstring extension was loaded and then using the correct function in our own code? Any thoughts?

[Minishlink]

I'm all with you about not using mbstring.func_overload, but sadly, people who use our libs have weird ideas sometimes.

I know that @Spomky adds symfony/polyfill-mbstring to his repos. That's one solution.

Another solution would be to replace every substr and strlen by safeSubstr and safeStrlen functions that call mb_substr/mb_strlen if they're defined.

What do you prefer?

[Spomky]

Hi @Minishlink ,

That is true that my repos use the symfony/polyfill-mbstring library, but I think I will remove this dependency for the next major revision.

I also agree with @fgrosse, the use of mbstring.func_overload should be avoided as it may create undesirable side effects and security issues.
As this library does not deal with multibyte strings, only binary ones, the use of the extension is not relevant and will decrease performance.

[fgrosse]

Thanks for your input. I decided to go with the safeSubstr, safeStrlen wrapper functions. Lets continue this issue at #64.

[fgrosse]

@Minishlink maybe you want to review #64 ? I also had a question about testing if the change actually fixes the described issue that you may be able to answer.

[Minishlink]

@fgrosse Will do asap, thanks!