Re: Inconsistency mbstring functions

On Mon, Dec 4, 2023 at 1:51 PM Stefan Schiller via internals <internals@lists.php.net> wrote: > > On Sat, Dec 2, 2023 at 6:13 AM 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 other mbstring developers have too. It dates > > back to the origin of the library; actually, even before the origin of mbstring, since mbstring was > > based on 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 libmbfl. Once upon a > > time, there was a text-processing library called libmbfl. libmbfl 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 generated by one > > routine would automatically be passed to the next. libmbfl could perform many wonderful > > text-processing tasks by substituting a different final filter at the end of the chain. > > > > But all was not well. Since libmbfl's filters processed text only one byte or > > codepoint at a time, and each routine had to save its state before returning, and restore its state > > upon entry, libmbfl was slow. Slow as a turtle, 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-entry 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 first > > byte. Then, text-processing tasks which were not dependent on the actual content of a string, but > > only on the number of codepoints, could be performed without ever invoking those wonderful, but > > painfully slow filters! libmbfl could skip through a string while just examining the first byte of > > each character. (Of course, this only worked for text encodings with an mblen_table.) For valid > > strings, the new method worked identically to the previous 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 interested in mbstring and > > reimplemented its internals, replacing the libmbfl code with fresh new code which ran many times > > faster. The new code was so much faster that in some cases, the mblen_table optimization actually > > became a pessimization! 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 Hyrum's Law > > (https://www.hyrumslaw.com/). With a huge body of legacy code relying on mbstring, almost any > > observable behavior change runs a significant risk of breaking someone's code. And when this > > happens, they will not hesitate 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 affected 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 strings 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 define > > 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. I think it is only a security issue when people accidentally think mb_* functions should be used if it is available. I've seen people do mb_strlen() on binary data, for example, not realizing the differences between mb_strlen and strlen. Or using mb_* functions and then passing them off to cryptographic functions. Robert Landers Software Engineer Utrecht NL