Page MenuHomePhabricator

BagOStuff should detect obsolete serialization or an unserialization resulting in a "wrong" object
Closed, DeclinedPublic

Description

Abstract

Following up from T156364: Warning: Empty regular expression in /srv/mediawiki/php-1.29.0-wmf.9/includes/parser/DateFormatter.php on line 200, when the signature of a php class changes, deploying the new version can result in some unexpected behavior. The problem arises when cached instances having the old signature are unserialized in the new environment.

Hence one can serialize to the cache a class that has a property foo marked public. When later the code is updated and the visibility of foo is made private, unserializing the old version produces an object having two foo property:

  • A public foo set by unserialize with the expected value
  • A private foo that comes from the new class definition and NULL

Trying to access foo PHP obey to the new class definition and retrieve the private one. Hence NULL instead of the value expected.

Repro

Mini case:

<?php
# Given two classes with the same property name but different visibility
class WithPublic {
    public $property;
    function __construct( $p ) { $this->property = $p; }
    function getProperty() { print $this->property; }
}
class WithPrivate {
    private $property;
    function __construct( $p ) { $this->property = $p; }
    function getProperty() { print $this->property; }
}


$priv = new WithPrivate( 'value' );

# Lets pretend it is held in a permanent cache, eg Memcached
$cache = serialize( $priv );

# Later on NEW code is deployed which change the property signature to public.
# Simulate the class changed:
$mut_priv_to_pub = unserialize(
    str_replace( '11:"WithPrivate"', '10:"WithPublic"', $cache) );


var_dump( $mut_priv_to_pub );
# class WithPublic#4 (2) {
#   public $property =>
#   NULL               <--- from class signature
#   private $property =>
#   string(5) "value"  <--- from unserialize()
# }

var_export( $mut_priv_to_pub->getProperty() );
# On Zend PHP: NULL
# On HHVM 3.15.4, 3.17.1: valueNULL
`

Details

Some relevant details extracted from T156364:

So that's probably where we're getting the weird behavior that makes it look like an invalid cache that goes away after an hour: rMW4ca09bd76f19: Make most of DateFormatter private changed how DateFormatter is serialized, so when 1.29.0-wmf.9 loads a DateFormatter from cache that was saved in 1.29.0-wmf.8 or vice versa it's probably missing setting of expected fields.
Assuming that's true, we could probably just redeploy and see that the errors really do go away after an hour. Or we could add a version number to the cache key like we do with some other classes that cache instances.


Interesting issue. PHP names private vars internally differently from public ones (since there can be a number of private vars with the same name in the inheritance chain). So we should be very careful moving to/from private in serialized classes.
Check out: https://3v4l.org/t2IPr


My summary is:
We had the spam after group0 and group1 deploy and that ceased after one hour. The same happens on group2 deploy, though I am not sure why we rollbacked four hours later since the spam should have been gone. Maybe due to another issue, but if the spam was still going on after the one hour TTL that confuses me.
Anomie is the huge asset and pointed to the non trivial issue which is that the cache unserialize objects that do not match the new class signature. Havoc happens because of PHP inconsistency.

That is at least the third time we have some havoc due to such signatures changes, I could not find previous bugs but I am 101% sure we had the exact same trouble and faced some kind of nice outage / blank pages as a result. Though it is really a corner case it is definitely annoying.

I like @Smalyshev https://3v4l.org/t2IPr it is rather annoying that an object can end up with two $a property, one public the other protected. I wish unserialize() would just explode in such case or warning and return false.

I went with my own one (P4826): that does public -> private and private -> public: https://3v4l.org/QKTGW That even show a bug/different behavior on HHMV which concatenate both properties (last line). Might be worth filling a bug https://github.com/facebook/hhvm/issues/7629 .

Would there be a way to have BagOfStuff to detect obsolete serialization or an unserialization resulting in a "wrong" object? Might be worth investigating further. I am not quite sure how that would work with array of objects or objects referencing other objects......

Details

Related Gerrit Patches:

Event Timeline

mmodell created this task.Jan 28 2017, 2:31 AM

The fancy solution:

  • Run some static analysis on classes as part of CI which would:
    • detect when the signature of a cached class changes
    • enforce that some component of the cache key must be altered every time the signature is changed.
mmodell triaged this task as Normal priority.Jan 28 2017, 2:36 AM
hashar updated the task description. (Show Details)Jan 28 2017, 9:54 AM
hashar updated the task description. (Show Details)Jan 28 2017, 10:01 AM

Imho unserialize() does the right thing. The class property get stored with a visibility and restored exactly the same way. The real trouble is that it should bails out when creating a property having the same name with a different visibility than the class contract.

I would expect unserialize() to return false and emit a warning about class mismatch or something like that.

Well, when you declare variable as "private" it means it - that means, outside of the class it's like it does not exist. Which means when you define property outside of the class, it's completely new one, not the same one. Check out: https://3v4l.org/GLP67

So moving variable to private is essentially the same as renaming it from $foo to $myclass_foo. unserialize() can't do much about it. At least not without generic "class change framework". Java has serialization IDs in classes for this, but PHP doesn't have anything so far.

Some thoughts:

  • If a field is removed, that should be ok.
  • If a field is added, that could be ok (e.g. a field that's lazy-loaded) or could be problematic (if it's not).
  • As already noted, a field with changed access is the same as a field being renamed. And a rename is equivalent to "field is removed" + "field is added".
  • Even if the field itself isn't changed, if the type of data stored in the field is changed (e.g. it changes from string to string[], or from a normal array to an associative array with the same values) that could still be a problem.
  • A completely generic solution (i.e. BagOStuff does all the work) would need to apply not only to objects directly serialized, but also to objects that are inside an array, or objects included by composition inside another object, etc. That seems like it would be hard to do without rewriting serialize() and unserialize().
  • A solution where each class is responsible for checking its own potential unserialization might use __wakeup() to check things (possibly just a version number check like Java's serialization IDs) and throw something on failure that BagOStuff would catch.

Something crazy that seems to actually work in quick testing (PHP 5.6.26, 7.0.15, and HHVM 3.12.11):

class SerializationIdCheckException extends RuntimeException {
    public function __construct( $class, $expected, $found ) {
        parent::__construct( "Serialization ID check failed in $class, $found !== $expected" );
    }
}

trait SerializationIdCheck {
    private $serializationIdCheck = self::SERIALIZATION_ID;

    public function __wakeup() {
        if ( $this->serializationIdCheck !== self::SERIALIZATION_ID ) {
            throw new SerializationIdCheckException( __CLASS__, self::SERIALIZATION_ID, $this->serializationIdCheck );
        }
        if ( is_callable( 'parent::__wakeup' ) ) {
            parent::__wakeup();
        }
    }
}

class Foo {
    use SerializationIdCheck;
    const SERIALIZATION_ID = "foo.1";
}

class Bar extends Foo {
    use SerializationIdCheck;
    const SERIALIZATION_ID = "bar.1";
}

$serialized = serialize( new Bar );

foreach ( [ 'none' => null, 'foo.1' => 'foo.0', 'bar.1' => 'bar.0' ] as $repl => $to ) {
    $test = $serialized;
    if ( $to !== null ) {
        // Fake loading from a different version by changing the ID in the serialized string
        $test = str_replace( serialize( $repl ), serialize( $to ), $test );
    }
    echo "Unserializing $test: ";
    try {
        unserialize( $test );
        echo "No exception thrown.\n";
    } catch ( SerializationIdCheckException $ex ) {
        echo "Caught exception: " . $ex->getMessage() . "\n";
    }
}

Looks reasonable. May slow down serialization a bit so I'd suggest a performance test but otherwise fine. One small tweak would be to add isset($this->serializationIdCheck) check to condition to weed out old versions having no check.

Of course, this would become trickier if the class in question already has its own __wakeup :) We don't have too many of them but Status and Title do have them. Then probably we could do something like:

class Title {
  use SerializationIdCheck { SerializationIdCheck::__wakeup as checkWakeup; }
  public function __wakeup() {
    $this->checkWakeup();
    // ...

  }
}

The isset() isn't useful, because if we're unserializing a version of the class that doesn't include the serializationIdCheck field then it's going to keep its default value. To catch that situation, we'd have to set $this->serializationIdCheck in the constructor instead of using a static initializer.

trait SerializationIdCheck {
    private $serializationIdCheck = 'unset';

    private function initializeSerializationIdCheck() {
        $this->serializationIdCheck = self::SERIALIZATION_ID;
    }

    /* same __wakeup method */
}

class Foo {
    use SerializationIdCheck;
    const SERIALIZATION_ID = "foo.1";

    public function __construct() {
        // Call parent::__construct() and whatever else needs doing
        $this->initializeSerializationIdCheck();
    }
}

I don't think there's any way to get initializeSerializationIdCheck() called automatically.

MaxSem renamed this task from BagOfStuff should detect obsolete serialization or an unserialization resulting in a "wrong" object to BagOStuff should detect obsolete serialization or an unserialization resulting in a "wrong" object.Jan 30 2017, 9:22 PM

Change 335101 had a related patch set uploaded (by MaxSem):
[WIP][POC] Automatic serialization guard trait

https://gerrit.wikimedia.org/r/335101

As for things that already have their own __wakeup, we might want to be careful that parent::__wakeup() doesn't get called twice and we'd probably also want to avoid parent::__wakeup() getting magically called by the trait instead of it being done where people can see it...

My above patch does the same thing as Brad's proposal, however it tracks the serialization format automatically, as opposed to manual maintenance.

Not sure why you need "unset" - if it's unset in the data and we expect it to be set, then it's broken.

The "unset" is there so that the exception message (if it isn't caught) is a bit clearer: "Serialization ID check failed in Class, unset !== 1234" rather than "Serialization ID check failed in Class, !== 1234".

Change 335101 abandoned by MaxSem:
[WIP][POC] Automatic serialization guard trait

Reason:
Point was demonstrated, those interested can pick up from that.

https://gerrit.wikimedia.org/r/335101

hashar added a subscriber: Bawolff.

That has hit us when deploying 1.32.0-wmf.20 (T203566).

Not a sub task per T203566#4564404. T203566 isn't required to settle for this task and vice-versa. This task is adding detection for category/source of problems. T203566 is an instance of such problem.

T231542 is a similar case with some AbuseFilter class stored in external storage. Changing the visibility of a property from public to protected cause the stored objects to no more be unserialized properly :-\

Krinkle closed this task as Declined.Aug 30 2019, 6:44 PM

Obsolete per Development policy banning the persistence of php-serialised data (or more specifically, unserialization of data retrieved from elsewhere).

There is a number of existing uses to be migrated (as time and resources permit), but investing in new ways to support it doesn't make sense at this time.