Make WordPress Core

Opened 10 years ago

Closed 9 years ago

#32279 closed defect (bug) (fixed)

Creating new posts/terms with non-latin characters is broken for tables with cp1251 collation in 4.1.5

Reported by: vloo's profile vloo Owned by: pento's profile pento
Milestone: 4.2.4 Priority: high
Severity: critical Version: 4.1.5
Component: Database Keywords: fixed-major
Focuses: Cc:

Description

The issue is identical to #32051.

Problematic code is in wp-includes/wp-db.php, as reverting it to 4.1.4 (just this file) solves the issue.

My guess (without having the time to check or prove it) is that changes in 4.2.2 were applied in 4.1.5 without considering that it was expected to be still tolerant to unorthodox collations.

P.S. Can't point out the version as it's still not in the list.

Attachments (3)

32279.diff (3.7 KB) - added by dd32 9 years ago.
32279.2.diff (4.1 KB) - added by mdawaffe 9 years ago.
32279.3.diff (4.1 KB) - added by mdawaffe 9 years ago.
Updated - clearer comment

Download all attachments as: .zip

Change History (29)

#1 @vloo
10 years ago

  • Version set to 4.1.5

cp1251_bulgarian_ci is problematic, I tested it on a site with cp1251_general_ci and it was alright there. It'd be helpful if someone else could also try to reproduce it.

#2 @SergeyBiryukov
10 years ago

  • Milestone changed from Awaiting Review to 4.2.3
  • Priority changed from normal to high
  • Severity changed from normal to critical

#3 @SergeyBiryukov
10 years ago

Confirmed. All branches from 3.7.8 to 4.2.2 appear to be affected.

Steps to reproduce:

  1. Get a Bulgarian or Russian package.
  2. Before installing, set DB_CHARSET to cp1251 in wp-config.php.
  3. Proceed with installing.

When you try to view the site after installing:

  • The default content is missing.
  • Most of the options in wp_options are missing.

When you try to log in:

  • It tells you that a DB upgrade is needed.
  • During the upgrade, there's a bunch of errors:
    WordPress database error: [Table 'wordpress.wp422_categories' doesn't exist]
    SELECT cat_ID, cat_name, category_nicename FROM wp422_categories
    
    WordPress database error: [Table 'wordpress.wp422_post2cat' doesn't exist]
    SELECT DISTINCT post_id FROM wp422_post2cat
    
    WordPress database error: [Unknown column 'post_category' in 'field list']
    SELECT ID, post_category FROM wp422_posts WHERE post_category != '0'
    
    WordPress database error: [Table 'wordpress.wp422_categories' doesn't exist]
    ALTER TABLE `wp422_categories` ADD INDEX ( `category_nicename` )
    
    WordPress database error: [Key column 'link_category' doesn't exist in table]
    ALTER TABLE `wp422_links` ADD INDEX ( `link_category` )
    
    WordPress database error: [Unknown column 'user_nickname' in 'field list']
    SELECT ID, user_nickname, user_nicename FROM wp422_users
    
    WordPress database error: [Table 'wordpress.wp422_categories' doesn't exist]
    SELECT * FROM wp422_categories ORDER BY cat_ID
    
    WordPress database error: [Table 'wordpress.wp422_post2cat' doesn't exist]
    SELECT post_id, category_id FROM wp422_post2cat GROUP BY post_id, category_id
    
    WordPress database error: [Table 'wordpress.wp422_linkcategories' doesn't exist]
    SELECT cat_id, cat_name FROM wp422_linkcategories
    
    WordPress database error: [Unknown column 'link_category' in 'field list']
    SELECT link_id, link_category FROM wp422_links
    

Previous release in each branch (3.7.7 to 4.2.1) works correctly.

This ticket was mentioned in Slack in #core by sergeybiryukov. View the logs.


10 years ago

#5 @pento
10 years ago

  • Milestone 4.2.3 deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #32165.

This is happening for the same reasons as described in 32165#comment:36.

@dd32
9 years ago

#6 @dd32
9 years ago

  • Milestone set to 4.2.3
  • Resolution duplicate deleted
  • Status changed from closed to reopened

I'm re-opening this as it seems like a different case than #32165 from what I can understand.

The issue at play here, from what I can see, is initially that the following query will always fail, as strip_text_from_query() will return '':

INSERT INTO table_with_cp1251_charset( `a` ) VALUES( 'safe data' )

The reason is that ultimately it performs this query:

LEFT( CONVERT( 'INSERT INTO table_with_cp1251_charset( `a` ) VALUES( \'safe data\' )' USING cp1251 ), 0 )

The LEFT( %s, 0 ) means it'll always fail, the 0 comes from the fact that strip_invalid_text() currently requires the length field be present when testing non-utf8/utf8mb4/latin1 strings, and strip_invalid_text_from_query() never passes that field.

Attached in 32279.diff is a unit test that demonstrates that, and a fix that passes the unit-test, but one that I'm not confident in - I'm struggling to find an example query that should fail.
As I understand it, the fix should be OK since the idea is that the CONVERT() query should fail, or return a different length of data, which causes the failure to get raised higher up in the chain.

I don't think this fixes all of the issues, I suspect #32165 will still kick in, but this allows the install & term creation to work again..

#7 @obenland
9 years ago

  • Owner set to dd32
  • Status changed from reopened to assigned

@dd32 is this something we should wait for pento to look at?

#8 @dd32
9 years ago

I've pinged @nbachiyski, @mdawaffe and @pento for a code review here, I haven't heard back yet from the first two. @pento suggested that it's likely that a bunch of the code in the function might not be needed, but couldn't give a yay or nay on the patch.

#9 @netweb
9 years ago

Related: #32917 Tests_DB_Charset tests don't fully cover wpdb::strip_invalid_text_for_column()

@mdawaffe
9 years ago

#10 @mdawaffe
9 years ago

Patch looks good to me. I fixed a bad variable assignment and added a couple more related (but slightly different) test cases in attachment:32279.2.diff.

@mdawaffe
9 years ago

Updated - clearer comment

#11 @mdawaffe
9 years ago

attachment:32279.3.diff is the same but clarifies a code comment in the tests.

#12 @pento
9 years ago

Let's go with 32279.3.diff for the moment. I recall there being some bigger issues with this area of code, related to #32165, but it's going to take me some time to get back up to speed. Worst case is that this is fixed in 4.2.3, and #32165 will be pushed back to 4.3.1.

#13 @pento
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 33310:

WPDB: ::strip_text_from_query() doesn't pass a length to ::strip_invalid_text(), which was causing queries to fail when they contained characters that needed to be sanity checked by MySQL.

Props dd32, mdawaffe, pento.

Fixes #32279.

#14 @pento
9 years ago

In 33311:

WPDB: ::strip_text_from_query() doesn't pass a length to ::strip_invalid_text(), which was causing queries to fail when they contained characters that needed to be sanity checked by MySQL.

Props dd32, mdawaffe, pento.

Merges [33310] to the 4.2 branch.

Fixes #32279.

#15 @SergeyBiryukov
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I can still reproduce the issue in 4.2.3 with the steps from comment:3, and I'm seeing a lot of similar reports on support forums from those using the cp1251 charset.

Steps to reproduce:

  1. Get a Bulgarian or Russian package.
  2. Before installing, set DB_CHARSET to cp1251 in wp-config.php.
  3. Proceed with installing.
  • When you try to view the site after installing, the default content is missing.
  • If you try to create a post or page, you'll always see _wp_posts_page_notice() instead of the editor, because get_default_post_to_edit() never succeeds due to __( 'Auto Draft' ) being translated into Cyrillic.
  • If installing with WP_DEBUG enabled, there's a bunch of notices in the process:
Notice: Undefined index: x_term_id in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_slug in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_term_group in wp-includes/wp-db.php on line 2751

Notice: Undefined index: x_post_author in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_date in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_date_gmt in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_content in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_excerpt in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_name in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_modified in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_modified_gmt in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_guid in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_comment_count in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_to_ping in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_pinged in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_content_filtered in wp-includes/wp-db.php on line 2751

Notice: Undefined index: x_comment_post_ID in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_comment_author_email in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_comment_author_url in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_comment_date in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_comment_date_gmt in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_comment_content in wp-includes/wp-db.php on line 2751

Notice: Undefined index: x_post_author in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_date in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_date_gmt in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_content in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_excerpt in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_name in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_modified in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_modified_gmt in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_guid in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_type in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_to_ping in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_pinged in wp-includes/wp-db.php on line 2751
Notice: Undefined index: x_post_content_filtered wp-includes/wp-db.php on line 2751
Last edited 9 years ago by SergeyBiryukov (previous) (diff)

#16 @dd32
9 years ago

  • Owner changed from dd32 to pento
  • Status changed from reopened to assigned

This ticket was mentioned in Slack in #core by obenland. View the logs.


9 years ago

#18 @pento
9 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 33455:

WPDB: When checking the encoding of strings against the database, make sure we're only relying on the return value of strings that were sent to the database. Also make sure that we're not trying to sanity check strings that've been marked as not needing sanity checking.

Fixes #32279.

#19 @pento
9 years ago

  • Keywords fixed-major added
  • Milestone changed from 4.2.3 to 4.2.4
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opening for potential inclusion in 4.2.4.

#20 @SergeyBiryukov
9 years ago

[33455] works as expected, +1 for merging into 4.2.4.

#21 @SergeyBiryukov
9 years ago

FWIW, 3.7.9 to 4.1.6 are also broken with the same results as in comment:3.

Can we merge both [33310] and [33455] there?

#22 @pento
9 years ago

Given that it was a bug in a security patch, I'm okay with going back to 4.1, as some people are still stuck there due to this bug.

I'm inclined to not patch 3.x, because the lack of this fix doesn't introduce new security issues.

#23 @pento
9 years ago

In 33476:

WPDB: When checking the encoding of strings against the database, make sure we're only relying on the return value of strings that were sent to the database. Also make sure that we're not trying to sanity check strings that've been marked as not needing sanity checking.

Merge of [33455] to the 4.2 branch.

See #32279.

#24 @pento
9 years ago

In 33479:

WPDB: ::strip_text_from_query() doesn't pass a length to ::strip_invalid_text(), which was causing queries to fail when they contained characters that needed to be sanity checked by MySQL.

Props dd32, mdawaffe, pento.

Merges [33310] to the 4.1 branch.

See #32279.

#25 @pento
9 years ago

In 33480:

WPDB: When checking the encoding of strings against the database, make sure we're only relying on the return value of strings that were sent to the database. Also make sure that we're not trying to sanity check strings that've been marked as not needing sanity checking.

Merge of [33455] to the 4.1 branch.

See #32279.

#26 @pento
9 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed
Note: See TracTickets for help on using tickets.