Hide (un)serialize data from ABI #2070
Conversation
The (UN)SERIALIZE_INIT/DESTROY macros now go through non-inlined functions, so any changes to them will apply to extensions without rebuilds. Additionally, the (un)serialize_data structures are now no longer exported. This means that we are allowed to change these structures in patch releases without breaking the ABI.
|
Motivation is to allow us to do changes like #2050 in a patch release (though that particular one can also be implemented without ABI break in a more complicated fassion, #2004). An example of a bug that likely cannot be fixed without ABI changes is https://bugs.php.net/bug.php?id=72785. I expect that we'll have more security fixes in the future where modifying unserialize data is necessary. |
|
+1 |
|
@dshafik Any word here? |
|
I presume this patch breaks the ABI? (but makes it so we won't have to in future) What's that going to mean for extension authors upgrading to 7.1? |
|
@dshafik Nothing, unless they manually accessed serialize/unserialize data (which is unlikely). This only means that extensions should be recompiled. |
|
@nikic in that case, I would be fine putting it in 7.1 given the amount of security issues lately. |
|
The idea looks good to me, also makes fixing bugs like https://bugs.php.net/bug.php?id=72785 easier. I think this should go into 7.1. |
|
Merged via f7caa2b into 7.1. |
The PHP source code for all versions of php 7 was keeping **pointers** to object properties, not copies of the object properties. There are two ways to fix this. This PR chooses the second. So does HHVM. - Keep copies of **all** zvals instead of pointers to zvals while unserializing, increment reference counts. - Defer calls to __wakeup until the very end. This requires copying only the objects of classes implementing __wakeup. hhvm handles __wakeup this way. https://github.com/facebook/hhvm/blob/2d5f00afbb033aec0cbc51cbe3a897af79cdcb28/hphp/runtime/base/variable-unserializer.cpp#L333 https://github.com/facebook/hhvm/blob/2d5f00afbb033aec0cbc51cbe3a897af79cdcb28/hphp/runtime/base/variable-unserializer.cpp#L965 Make the publicly exposed APIs var_unserialize and var_unserialize_ex perform this finalization at the very end. Many extensions use var_unserialize to unserialize data (e.g. ext/session, or extensions outside of php-src), and this change is made in a way that shouldn't break them. - Move the inner zval deserialization code into a static method, so that they won't accidentally be called and miss a call to __wakeup. - Fix merge conflicts with php#2070
The PHP source code for all versions of php 7 was keeping **pointers** to object properties, not copies of the object properties. There are two ways to fix this. This PR chooses the second. So does HHVM. - Keep copies of **all** zvals instead of pointers to zvals while unserializing, increment reference counts. - Defer calls to __wakeup until the very end. This requires copying only the objects of classes implementing __wakeup. hhvm handles __wakeup this way. https://github.com/facebook/hhvm/blob/2d5f00afbb033aec0cbc51cbe3a897af79cdcb28/hphp/runtime/base/variable-unserializer.cpp#L333 https://github.com/facebook/hhvm/blob/2d5f00afbb033aec0cbc51cbe3a897af79cdcb28/hphp/runtime/base/variable-unserializer.cpp#L965 Make the publicly exposed APIs var_unserialize and var_unserialize_ex perform this finalization at the very end. Many extensions use var_unserialize to unserialize data (e.g. ext/session, or extensions outside of php-src), and this change is made in a way that shouldn't break them. - Move the inner zval deserialization code into a static method, so that they won't accidentally be called and miss a call to __wakeup. - Fix merge conflicts with php#2070
|
"This only means that extensions should be recompiled." Don't we need a PHP API version bump in 7.1 now then? Currently it hasn't been bumped from 7.0 |
|
@nikic can you confirm and please ensure this is done for RC2 if necessary. |
|
PHP 7.0: PHP 7.1: So API for 7.1 has already been bumped. I think we only do this once per release (when the first API change is done), but not sure. |
|
Yes, we only bump once per release. However, I meant the PHP API version, not the Zend Extension one. This one hasn't been bumped from 7.0 |
The (UN)SERIALIZE_INIT/DESTROY macros now go through non-inlined
functions, so any changes to them will apply to extensions without
rebuilds.
Additionally, the (un)serialize_data structures are now no longer
exported.
This means that we are allowed to change these structures in patch
releases without breaking the ABI.