Make WordPress Core

Opened 6 years ago

Last modified 2 months ago

#51486 new defect (bug)

The add_option function should not be able to update existing rows in the database.

Reported by: khag7 Owned by:
Priority: normal Milestone: Awaiting Review
Component: Database Version: 2.9
Severity: normal Keywords: has-patch has-unit-tests
Cc: Focuses:

Description

In certain edge cases, add_option is able to update existing option values. This should not be possible. If SQL is executed which instructs "Insert XYZ" into database, then the insert should fail if the key already exists in the database, unless the SQL specifies that an UPDATE should happen.

The add_option function does check first to see if an option exists before attempting to add it to the database. In the overwhelming majority of cases, this works fine because when the option exists the function will return early before attempting to insert into the database.

However, consider a scenario where an object cache is being used. And further consider that the object cache may report incorrectly. If the object cache tells add_option that the value doesn't exist in the database (but in reality it does!), then add_option continues on and attempts to insert the supposedly new option.

In that particular circumstance, the SQL query, shown below, is executed which will insert the new row. And in the event that the option already exists, it will be overwritten.

The issue here is that add_option shouldn't be able to update the existing value. The query should fail. Why is the ON DUPLICATE KEY UPDATE clause included here?

<?php    // Code here is from wp-includes/option.php Line 581:
    $result = $wpdb->query(
        $wpdb->prepare(
            "INSERT INTO `$wpdb->options` (`option_name`, `option_value`, `autoload`)
             VALUES (%s, %s, %s)
             ON DUPLICATE KEY UPDATE
                `option_name`  = VALUES(`option_name`),
                `option_value` = VALUES(`option_value`),
                `autoload`     = VALUES(`autoload`)
            ",
            $option,
            $serialized_value,
            $autoload
        )
    );

I realize this is an edge case. If the object cache being used gives bad information to add_option then really its the object caching plugin at fault. However, I can't understand a possible circumstance where add_option should ever UPDATE an existing option. The SQL here should not include ON DUPLICATE KEY UPDATE. Am I missing something? Is there a good reason for that clause? What would happen if that clause were removed?

Change History (5)

#1 @khag7
6 years ago

And now for a little history...

It was 11 years ago, and add_options was located in wp-includes/functions.php back then.

The clause ON DUPLICATE KEY UPDATE was added to the add_option function in ticket #11437 and commit [12403]

The description in that ticket does explain why this clause exists in the SQL. On a busy site, you can't know whether the option exists or not before attempting to insert it. As such, an INSERT query may fail.

Unfortunately, they solved that ticket incorrectly. The solution should have been that whatever piece of code was attempting to add the option site_transient_timeout_theme_roots should have been using update_option instead. This would've given them the result they wanted. Instead, they patched add_option with ON DUPLICATE KEY UPDATE and now we have a function that is intended to add and never update, but which in fact does update.

#2 @markparnell
6 years ago

  • Component GeneralDatabase
  • Keywords needs-patch needs-unit-tests added
  • Version2.9

This ticket was mentioned in PR #12065 on WordPress/wordpress-develop by @sukhendu2002.


3 months ago
#4

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

@yusufmudagal commented on PR #12065:


2 months ago
#5

Tested the patch locally against current origin/trunk in a disposable worktree.

Results:

  • php -l src/wp-includes/option.php passed.
  • vendor/bin/phpunit --filter test_add_option_should_not_update_existing_option_when_notoptions_cache_is_stale passed: 1 test, 7 assertions.

This covers the stale notoptions cache regression path added in the PR.

Note: See TracTickets for help on using tickets.