Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:121920
MIME-Version: 1.0
References: <CAMZNyFfiP5y8QAcNBCo235xG30Ra-PgCKcYfsZVuvgwy9Re-8w@mail.gmail.com>
 <CAFPFaMJeLD1WVzczjfXa4iKg0QtgKxhH0e3mzOMjsa9POf=C9w@mail.gmail.com> <CACsECNcbe+D=8WMa=LG-ts6_Rq1W91hneb43i07L58xC5WUt3w@mail.gmail.com>
In-Reply-To: <CACsECNcbe+D=8WMa=LG-ts6_Rq1W91hneb43i07L58xC5WUt3w@mail.gmail.com>
Reply-To: Stefan Schiller <stefan.schiller@sonarsource.com>
Date: Mon, 4 Dec 2023 13:51:30 +0100
Message-ID: <CAMZNyFe0YywB5WGt3ga6s1SFOytNLFfF3=eEfMhnpWTDh24C9w@mail.gmail.com>
To: Alex <alexinbeijing@gmail.com>
Cc: "G. P. B." <george.banyard@gmail.com>, internals@lists.php.net
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Subject: Re: [PHP-DEV] Inconsistency mbstring functions
From: internals@lists.php.net ("Stefan Schiller via internals")

On Sat, Dec 2, 2023 at 6:13=E2=80=AFAM Alex <alexinbeijing@gmail.com> wrote=
:
>
> Dear Stefan, and Dear Gina,
>
> Thanks for the message. Yes, Stefan has rediscovered an interesting quirk=
 of the mbstring library. I have been aware of this for a long time, and ot=
her mbstring developers have too. It dates back to the origin of the librar=
y; actually, even before the origin of mbstring, since mbstring was based o=
n another library called libmbfl, and this behavior originates from libmbfl=
.
>
> Pull your chair up around the fire and let me tell you the tale of libmbf=
l. Once upon a time, there was a text-processing library called libmbfl. li=
bmbfl was based on a collection of text-decoding routines (which converted =
bytes to codepoints) and text-encoding routines (which converted codepoints=
 to bytes). Each such routine was structured as a stateful "filter". These =
filters could be assembled into "chains", whereby the output values generat=
ed by one routine would automatically be passed to the next. libmbfl could =
perform many wonderful text-processing tasks by substituting a different fi=
nal filter at the end of the chain.
>
> But all was not well. Since libmbfl's filters processed text only one byt=
e or codepoint at a time, and each routine had to save its state before ret=
urning, and restore its state upon entry, libmbfl was slow. Slow as a turtl=
e, slow as a snail, slow as whatever-slowly-moving-thing-you-can-think-of. =
Oh, what was libmbfl to do? A clever plan was hatched: give libmbfl a 256-e=
ntry table called a "mblen_table" for each supported text encoding with the=
 property that the byte length of a character can be determined from its fi=
rst byte. Then, text-processing tasks which were not dependent on the actua=
l content of a string, but only on the number of codepoints, could be perfo=
rmed without ever invoking those wonderful, but painfully slow filters! lib=
mbfl could skip through a string while just examining the first byte of eac=
h character. (Of course, this only worked for text encodings with an mblen_=
table.) For valid strings, the new method worked identically to the previou=
s one. For invalid strings, there were significant differences in behavior,=
 but libmbfl tried to ignore these and bravely pressed on.
>
> The story ends with an ironic twist. Many years later, I became intereste=
d in mbstring and reimplemented its internals, replacing the libmbfl code w=
ith fresh new code which ran many times faster. The new code was so much fa=
ster that in some cases, the mblen_table optimization actually became a pes=
simization! In other cases, the mblen_table-based code is still faster, but=
 not by a large amount. But now mbstring was haunted by the spectre of Hyru=
m's Law (https://www.hyrumslaw.com/). With a huge body of legacy code relyi=
ng on mbstring, almost any observable behavior change runs a significant ri=
sk of breaking someone's code. And when this happens, they will not hesitat=
e to vent their rage on the hapless maintainers.
>
> Notwithstanding the rage of the users, about a year ago, I did remove the=
 mblen_table-based code in one place where benchmarks clearly showed it was=
 acting as a pessimization. I don't remember which mbstring function was af=
fected and would need to check the commit log to confirm.

Hi Alex,

Thank you very much for sharing this background context.

>
> Personally, I think the real issue here is not the inconsistency between =
mbstring functions which are based on the mblen_tables and those which are =
not. I think a lot of mbstring operations should not be used on invalid str=
ings at all, and that for such operations, mbstring would do well to throw =
an exception if it receives invalid input. (Like mb_strpos; how do you defi=
ne the "position of a UTF-8 substring" when the parent string is not UTF-8 =
at all?) But that would be a huge BC break.
>

My biggest concern is that this quirk can cause security issues in
user code. I came across this in the first place when discovering an
exploitable security vulnerability in an application. From my point of
view, this is not only about inconsistent behavior but also violates
the documentation for specific functions like mb_strstr. I agree that
a lot of mbstring operations should not be used on invalid strings,
and an exception seems to be an appropriate answer despite the huge BC
impact.

Best Regards,
Stefan Schiller