sync #2
Merged
sync #2
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.
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.
…le input encodings Thanks to Kamil Tekiela for pointing out that there was no test case for this.
In e245985, I combined mbstring's "SJIS-win" text encoding into CP932. This was done after doing some testing which appeared to show that the mappings for "SJIS-win" were the same as those for "CP932". Later, it was found that there was actually a small difference prior to e245985 when converting Unicode to CP932. The mappings for the following two codepoints were different: CP932 SJIS-win U+203E 0x7E 0x81 0x50 U+00A5 0x5C 0x81 0x8F As shown, mbstring's "CP932" mapped Unicode's 'OVERLINE' and 'YEN SIGN' to the ASCII bytes which have conflicting uses in most legacy Japanese text encodings. "SJIS-win" mapped these to equivalent JIS X 0208 fullwidth characters. Since e2459867af was not intended to cause any user-visible change in behavior, I am rolling back the merge of "CP932" and "SJIS-win". It seems doubtful whether these two text encodings should be kept separate or merged in a future release. An extensive discussion of the related historical background and compatibility issues involved can be found in this GitHub thread: #8308
* PHP-8.1: Reintroduce legacy 'SJIS-win' text encoding in mbstring
* PHP-8.0: Prepare for 8.0.24
* PHP-8.1: Prepare for 8.0.24
While the reason-phrase in a HTTP response status line is usually short, there is no actual limit specified by the RFCs. As such, we must not assume that the line fits into the buffer (which is currently 128 bytes large). Since there is no real need to present the complete status line, we simply read and discard the rest of a long line. Co-authored-by: Tim Düsterhus <timwolla@googlemail.com> Closes GH-9319.
* PHP-8.0: Fix GH-9316: $http_response_header is wrong for long status line
github-actions
bot
added
Extension: random
Extension: readline
Extension: reflection
Extension: session
Extension: shmop
Extension: simplexml
Extension: snmp
Extension: soap
Extension: sockets
Extension: sodium
Extension: spl
Extension: sqlite3
Extension: standard
Extension: sysvmsg
Extension: sysvsem
Extension: sysvshm
Extension: tidy
Extension: tokenizer
Extension: xml
Extension: xmlreader
Extension: xmlwriter
Extension: xsl
Extension: zend_test
Extension: zip
Extension: zlib
SAPI: cgi
SAPI: cli
SAPI: fpm
SAPI: phpdbg
labels
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
19 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.
No description provided.