Major overhaul of mbstring (part 25) #9303
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…handler When micro-benchmarking on relatively short ASCII strings, the new implementation was about 30% faster than the old one.
Fuzzing revealed that something was missed here when making the new encoding conversion code match the behavior of the old code. In the next major release of PHP, support for these non-encodings will be dropped, but in the meantime, it is better to match the legacy behavior.
In 04e59c9, I amended the UTF-8 conversion code, so that when given invalid input, it would emit a number of errors markers harmonizing with the WHATWG's specification of the standard UTF-8 decoding algorithm. (Which, gentle reader of commit logs, you can find online at https://encoding.spec.whatwg.org/#utf-8-decoder.) However, the code in 04e59c9 was faulty in the case that a truncated UTF-8 code unit starts with 0xF1. Then, in dc1ba61, when making a small refactoring to a different part of the UTF-8 conversion code, I inexplicably broke part of the working code, causing the same fault which was already present with truncated UTF-8 code units starting with 0xF1 to also occur with 0xF2 and 0xF3 as well. I don't remember what inane thoughts I was thinking when I pulled off this feat of utter mental confusion. None of these cases were covered by unit tests, by the way. Thankfully, my trusty fuzzer picked up on this when testing the new implementation of mb_parse_str (since the legacy UTF-8 conversion filter did not suffer from the same problem, and I was fuzzing to find any differences in behavior between the old and new implementations). Fortuitously, the fuzzer also picked up another issue which was present in 04e59c9. I was emitting only one error marker for truncated code units starting with 0xE0 or 0xED, in cases where the WHATWG standard indicates two should be emitted. Examples are 0xE0 0x9F <END OF STRING> or 0xED 0xA0 <END OF STRING>. Code units starting with 0xE0-0xED should have 3 bytes. If the first byte is 0xE0, the second MUST be 0xA0 or greater. (Otherwise, the codepoint could have fit in a two-byte code unit.) And if the first byte is 0xED, the second MUST be 0x9F or less. According to the WHATWG algorithm, step 4, if the second byte is outside the legal range, then the decoder should emit an error... AND reprocess the out-of-range byte. The reprocessing will then cause another error. That's why the decoder should indicate two errors and not one.
…pe sequence Fuzzing revealed a small difference between the number of error markers which the legacy ISO-2022-JP and JIS7/8 conversion code emitted for truncated escape sequences and those emitted by the new code. The behavior of the old code seems more reasonable here, so we will imitate it.
…gacy implementation The legacy Base64 conversion code in mbstring automatically wrapped the output to 72 columns, and the new code imitates this behavior. Frankly, I'm not sure if this is a good idea or not (people could easily manually wrap it if they want to), but have stuck with this behavior for backwards compatibility. However, fuzzing revealed one case where we were not wrapping to 72 columns; if the input string is not a multiple of 3 characters, meaning that the output must be padded, and the point where we must add the final (padded) output happens to be just beyond 72 columns.
CP50220 converts some codepoints which represent kana (hiragana/katakana) to a different form. This is the only difference between CP50220 and CP50221 (which doesn't perform such conversion). In some cases, this conversion means collapsing two codepoints to a single output byte sequence. Since the legacy text conversion filters only worked a byte at a time, the legacy filter had to cache a byte, then wait until it was called again with the next byte to compare the cached byte with the following one. That was all fine, but it didn't work as intended when there were errors (invalid byte sequences) in the input. Our code (both old and new) for emitting error markers recursively calls the same conversion filter. When the old CP50220 filter was called recursively, the logic for managing cached bytes did not behave as intended. As a result, the error markers could be reordered with other characters in the output. I used an ugly hack to fix this in 6938e35; when making a recursive call to emit an error marker, temporarily swap out `filter->filter_function` to bypass the byte-caching code, so the error marker immediately goes through to the output. This worked, but I overlooked the fact that the very same problem can occur if an invalid byte sequence is detected *in the flush function*. Apply the same (ugly) fix.
EUC-JP-2004 includes special byte sequences starting with 0x8E for kana. The legacy output routine for EUC-JP-2004 emits these sequences if the value of the output variable `s` is between 0x80 and 0xFF. Since the same routine was also used for SJIS-2004 and ISO-2022-JP-2004, before 8a915ed, the same 0x8E sequences would be emitted when converting to those text encodings as well. But that is completely wrong. 0x8E 0x__ does not mean the same in SJIS-2004 or ISO-2022-JP-2004 as it does in EUC-JP-2004. Therefore, in 8a915ed, I fixed the legacy conversion routine by checking whether the output encoding is EUC-JP-2004 or not. If it's not, and `s` is 0x80-0xFF, I made it emit an error. Well, it turns out that single bytes with values from 0xA1 to 0xDF are meaningful in SJIS-2004. To emit these bytes when appropriate, I had to amend the legacy conversion routine again. (For clarity, this does NOT mean reverting to the behavior prior to 8a915ed. We were right not to emit sequences starting with 0x8E in SJIS-2004. But in SJIS-2004, we *do* sometimes need to emit single bytes from 0xA1-0xDF.)
…tion Up until now, I believed that mbstring had been designed such that (legacy) text conversion filter objects should not be re-used after the 'flush' function is called to complete a text conversion operation. However, it turns out that the implementation of _php_mb_encoding_handler_ex DID re-use filter objects after flush. That means that functions which were based on _php_mb_encoding_handler_ex, including mb_parse_str and php_mb_post_handler, would break in some cases; state left over from converting one substring (perhaps a variable name) would affect the results of converting another substring (perhaps the value of the same variable), and could cause extraneous characters to get inserted into the output. All this code should be deleted soon, but fixing it helps me to avoid spurious failures when fuzzing the new/old code to look for differences in behavior.
|
Sorry, there is a test fail here. Will fix it up tomorrow. |
alexdowad
force-pushed
the
cleanup-mbstring-25
branch
2 times, most recently
from
2e47d8a to
86ee71f
Compare
The use of a special 'vtbl' for converting between '7bit' and '8bit' text meant that '7bit' text would not be converted to wchars before going to '8bit'. This meant that the special value MBFL_BAD_INPUT, which we use to flag an erroneous byte sequence in input text (and which is required by functions like mb_check_encoding), would pass directly to the output, instead of being converted to the error marker specified by mb_substitute_character. This issue dates back to the time when I removed the mbfl 'identify filters' and made encoding validity checking and encoding detection rely only on the conversion filters.
…sequence SJIS-Mobile#SOFTBANK text encoding supports special escape sequences, which shift the decoder into a mode where each single byte represents an emoji. To get out of this mode, a 0xF (SHIFT OUT) byte can be used. After one of these special escape sequences, the new conversion code expected to see at least one more byte. However, there doesn't seem to be any particular reason why it should be treated as an error condition if a string ends abruptly after one of these escapes. Well, the escape sequence is useless in that case, but it is a complete and valid escape sequence. The legacy conversion code did allow a string to end immediately after one of these escape sequences. Amend the new code to allow the same.
• The legacy conversion code did not emit an error marker if an escape sequence was truncated. • BOTH old and new conversion code would shift from KSC5601 (KS X 1001) mode to ASCII mode on an invalid escape sequence. This doesn't make any sense.
Because this routine used a signed char buffer to hold the bytes in a (possible) HTML entity, any bytes with the MSB set would be sign-extended when converting to int; for example, 0x86 would become 0xFFFFFF86 (or -121). Codepoints with huge values, like 0xFFFFFF86, are not valid and if any were passed to the output filter, it would treat them as errors and emit error markers.
alexdowad
force-pushed
the
cleanup-mbstring-25
branch
from
86ee71f to
16ea44b
Compare
|
OK, ready for review now. Thanks. |
|
Thanks for this. I don't see any issues with this PR, but I can't verify that the logic is correct without studying the conversion rules. But it looks correct. Maybe someone else wants to take a brief look too. |
Thanks very much for reviewing! |
|
@kamil-tekiela There are no dumb questions.
I'm not at my PC right now, but `$array = []; mb_parse_str('abc=def',
$array);` should work. If not please let me know.
…On Fri., Aug. 12, 2022, 4:58 p.m. Kamil Tekiela, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In ext/mbstring/mb_gpc.c
<#9303 (comment)>:
> @@ -255,6 +243,7 @@ const mbfl_encoding *_php_mb_encoding_handler_ex(const php_mb_encoding_handler_i
if (identd != NULL) {
n = 0;
while (n < num) {
+ mbfl_string string;
Sorry for asking dumb questions, but which test? Or can you give me a MCVE
that would allow me to set a breakpoint?
—
Reply to this email directly, view it on GitHub
<#9303 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAIESX3UXBMCGOPQKIG3F7TVYZRBNANCNFSM56JJJ7ZQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
This doesn't work for me. Each time I get |
Ah, I'm sorry. Could you add |
…le input encodings
✅ Can’t say much about the C code |
|
Thanks very much, @ausi. |
cmb69
approved these changes
Aug 16, 2022
|
Merged. Thanks all very much for the reviews. It was appreciated. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The saga continues.
FYI @cmb69 @nikic @kamil-tekiela
Perhaps @ausi might be interested? Part of this is related to the issue he raised in #8360.