Make WordPress Core

Opened 6 years ago

Closed 11 months ago

Last modified 11 months ago

#11175 closed defect (bug) (duplicate)

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

Reported by: sirzooro Owned by: hakre
Milestone: Priority: normal
Severity: normal Version: 2.9
Component: Charset Keywords: has-patch tested needs-unit-tests dev-feedback
Focuses: Cc:


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().

Attachments (1)

formatting.php.diff (557 bytes) - added by sirzooro 6 years ago.

Download all attachments as: .zip

Change History (20)

@sirzooro6 years ago

comment:1 @miqrogroove6 years ago

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.

comment:2 @miqrogroove6 years ago

  • Keywords tested added; needs-testing removed

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.

comment:3 @westi6 years ago

  • Keywords needs-unit-tests added

Moved to 3.0 for now.

Need to add some unit tests for this.

comment:4 @nacin6 years ago

  • Milestone changed from 2.9 to 3.0


comment:5 @hakre6 years ago

  • Component changed from General to Charset
  • Owner set to hakre

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

comment:6 @hakre6 years ago

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.

comment:7 @hakre6 years ago

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

comment:8 @hakre6 years ago

Related: #10543

comment:9 @hakre6 years ago

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.

comment:10 @hakre6 years ago

Related: #11619

comment:11 @westi5 years ago

  • Milestone changed from 3.0 to Future Release

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.

comment:12 @miqrogroove5 years ago

It is already tested as noted above.

comment:14 @hakre5 years ago

Related: #14618

comment:16 in reply to: ↑ 15 @westi4 years ago

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.

comment:17 @chriscct711 months ago

  • Keywords dev-feedback added

@westi and @nacin what are your thoughts about continuing with this ticket? Should it be maybelatered?

comment:18 @askapache11 months ago

  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #29717.

comment:19 @DrewAPicture11 months ago

  • Milestone Future Release deleted
Note: See TracTickets for help on using tickets.