wp_check_invalid_utf8() should drop invalid utf-8 chars only instead of truncating string

[sirzooro]

When you call wp_check_invalid_utf8() with 2nd param set to true, it tries to strip invalid utf-8. Now it removes 1st invalid utf-8 char and all chars after it, no matter if they are correct or not. Additionally it can print following notice:

Notice: iconv() [function.iconv]: Detected an illegal character in input string in .../wp-includes/formatting.php on line 437

Attached patch changes this, so function removes invalid chars only. Additionally it is less configuration-dependent, because it can use either mb_convert_encoding() or iconv().

When testing, please ensure this function never removes byte values less than 128. A situation where the function sees a 2-byte marker and then underflows, deletes two bytes and returns the results, would lead to code injection problems.

iconv and mb_convert_encoding behave identically. Tested with various valid and invalid byte sequences. These functions do not check for code point validity, but they do a good job of stripping individual bytes that fail to decode.

Trivially noted disagreement with preg_match using invalid code points 0xD800 and 0x140000.

Moved to 3.0 for now.

Need to add some unit tests for this.

Moving.

Reviewed the patch, looks good to me. Thanks for it, that really is an improvement. iconv rocks.

I think it's noteworthy to remind that wp_check_invalid_utf8() should not be used to check wether or not a string is a valid utf8 string. The function can lead to false results because it's broken.

Use seems_utf8() to verify a string beign properly UTF8 encoded or not.

seems_utf8() is broken as well, reference: #5998

Related: #10543

Since nor mb_string not iconv are non-default, I think there should be a fallback regex as well (I should be able to provide one w/o the /u modifier). So the function savely behaves on all installs.

To select which function to prefer, a shootout between all options should be run to checkout which one of those performs fastest.

Related: #11619

This is too risky to do without alot of testing.

Truncating is usually a better response to invalid utf8 because it is very hard to determine what the real cause of invalidity is and where it is safe to start parsing the bytestream again.

Moving to future release until we have some detail tests for this.

It is already tested as noted above.

Related: #15827

Related: #14618

See ​http://gsoc.svn.wordpress.org/2011/jakub.tyrcha for unit tests

Replying to jakub.tyrcha:

See ​http://gsoc.svn.wordpress.org/2011/jakub.tyrcha for unit tests

Please could you create a ticket in the ​unit-tests trac and attach the patch there too.