Skip to content

date: Fix unserialization of Time\Duration #23629

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
TimWolla wants to merge 1 commit into
base: master
Choose a base branch
from
+137 −6
Open

date: Fix unserialization of Time\Duration #23629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ PHP NEWS
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
?? ??? ????, PHP 8.6.0RC1

- Date:
. Fix unserialization of Time\Duration. (timwolla)

- DOM:
. Fixed use-after-free when re-constructing a DOMXPath whose php:function
registrations are freed while still reachable from the cycle collector.
Expand Down
63 changes: 63 additions & 0 deletions ext/date/tests/time/duration/serialize.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
--TEST--
Time\Duration: serialize()
--FILE--
<?php

require __DIR__ . '/helper.inc';

var_dump($serialized = serialize(Time\Duration::fromSeconds(1, 2)->negate()));
echo f($unserialized = unserialize($serialized)), PHP_EOL;
var_dump(serialize($unserialized));
echo f($unserialized->add($unserialized)), PHP_EOL;

try {
// $negative is not bool.
unserialize('O:13:"Time\Duration":3:{s:7:"seconds";i:1;s:11:"nanoseconds";i:1;s:8:"negative";i:1;}');
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

try {
// $seconds is negative.
unserialize('O:13:"Time\Duration":3:{s:7:"seconds";i:-1;s:11:"nanoseconds";i:1;s:8:"negative";b:0;}');
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

try {
// Dynamic property.
unserialize('O:13:"Time\Duration":4:{s:7:"seconds";i:1;s:11:"nanoseconds";i:1;s:8:"negative";b:0;s:3:"foo";N;}');
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}


try {
// Out of range nanoseconds
unserialize('O:13:"Time\Duration":3:{s:7:"seconds";i:1;s:11:"nanoseconds";i:1000000000;s:8:"negative";b:0;}');
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

try {
Time\Duration::fromSeconds(1, 1)
->__unserialize([
'seconds' => 2,
'nanoseconds' => 2,
'negative' => true,
]);
} catch (Throwable $e) {
echo $e::class, ': ', $e->getMessage(), PHP_EOL;
}

?>
--EXPECT--
string(85) "O:13:"Time\Duration":3:{s:7:"seconds";i:1;s:11:"nanoseconds";i:2;s:8:"negative";b:1;}"
-1.000000002
string(85) "O:13:"Time\Duration":3:{s:7:"seconds";i:1;s:11:"nanoseconds";i:2;s:8:"negative";b:1;}"
-2.000000004
Exception: Invalid serialization data for Time\Duration object
Exception: Invalid serialization data for Time\Duration object
Error: Cannot create dynamic property Time\Duration::$foo
Exception: Invalid serialization data for Time\Duration object
Error: Cannot modify readonly property Time\Duration::$seconds
4 changes: 4 additions & 0 deletions ext/date/time.stub.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ private function __construct()
{
}

public function __unserialize(array $data): void
{
}

public static function fromSeconds(int $seconds, int $nanoseconds = 0): Duration
{
}
Expand Down
8 changes: 7 additions & 1 deletion ext/date/time_arginfo.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

65 changes: 60 additions & 5 deletions ext/date/time_duration.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,20 +77,24 @@ static inline php_date_time_duration *create_duration_shell(zval *target)
return Z_DATE_TIME_DURATION_P(target);
}

ZEND_ATTRIBUTE_NODISCARD static inline zend_result sync_properties(php_date_time_duration *object)
static inline bool duration_representable(const timelib_duration *duration)
{
if (
return
/* Check if the duration would overflow the $seconds property. */
object->duration.seconds > ((uint64_t)ZEND_LONG_MAX)
duration->seconds <= ((uint64_t)ZEND_LONG_MAX)
/* This constraint is an explicit part of PHP's API: It is the maximum $seconds
* value that allows storing the entire duration as a single int64_t counting
* nanoseconds, which might be desirable in the future when userland `int` is
* consistently 64 bits.
*
* While it is currently also enforced by timelib, this might change
* in a future version of timelib, thus we also enforce it manually. */
|| object->duration.seconds > UINT64_C(9223372035)
) {
&& duration->seconds <= UINT64_C(9223372035);
}

ZEND_ATTRIBUTE_NODISCARD static inline zend_result sync_properties(php_date_time_duration *object)
{
if (!duration_representable(&object->duration)) {
throw_out_of_range_exception();
return FAILURE;
}
Expand Down Expand Up @@ -149,6 +153,57 @@ PHP_METHOD(Time_Duration, __construct)
zend_throw_error(NULL, "Cannot directly construct Time\\Duration, use Time\\Duration::from*() methods instead");
}

PHP_METHOD(Time_Duration, __unserialize)
{
php_date_time_duration *duration = Z_DATE_TIME_DURATION_P(ZEND_THIS);

HashTable *data;

ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_ARRAY_HT(data);
ZEND_PARSE_PARAMETERS_END();

/* object_properties_load() handles readonly and dynamic properties. */
object_properties_load(&duration->std, data);
if (EG(exception)) {
RETURN_THROWS();
}

zval *seconds = OBJ_PROP_NUM(&duration->std, 0);
zval *nanoseconds = OBJ_PROP_NUM(&duration->std, 1);
zval *negative = OBJ_PROP_NUM(&duration->std, 2);
/* object_properties_load() does not type check. We need to do this ourselves. */

@TimWolla TimWolla Sep 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is #9708.

if (Z_TYPE_P(seconds) != IS_LONG || Z_TYPE_P(nanoseconds) != IS_LONG || (Z_TYPE_P(negative) != IS_FALSE && Z_TYPE_P(negative) != IS_TRUE)) {
zend_throw_exception_ex(NULL, 0, "Invalid serialization data for %s object", ZSTR_VAL(duration->std.ce->name));

@ndossche ndossche Sep 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you typed check, after the properties have been overwritten, but that leaves the object in an unexpected type-violating state AFAICT?

@ndossche ndossche Sep 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep in mind people can call ->__unserialize manually BTW.

@TimWolla TimWolla Sep 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but that leaves the object in an unexpected type-violating state AFAICT?

That is correct, but I don't think this situation is observable in any way: Duration is a final readonly internal class, thus the engine prevents constructing objects without going through the constructor (or unserialization).

There are thus two cases to reach __unserialize():

  1. Manually calling ->__unserialize() on an existing object: This is guaranteed to fail due to object_properties_load() rejecting the reassignment of the readonly properties.
  2. Unserializing a payload.

In the second case, the types could mismatch after object_properties_load(), but I don't think there is a way this broken object can be observed. It will cleanly be destructed at the end of the failed unserialize() call (which is safe since Duration doesn't have a custom destructor and particularly no destructor that accesses the properties).

@ndossche ndossche Sep 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that's wrong, because destructors of other objects can run at arbitrary times. This was fun to make:

<?php
class kaboem {
    public $f;
    public function __destruct() {
        $GLOBALS['hier'] = $this->f;
    }
}

$payload = 'O:13:"Time\Duration":3:{s:7:"seconds";O:6:"kaboem":1:{s:1:"f";r:1;}s:11:"nanoseconds";i:1;s:8:"negative";b:0;}';
try {
    unserialize($payload);
} catch (Throwable $e) {
    echo get_class($e), ': ', $e->getMessage(), PHP_EOL;
}
$e = null; // to GC buffer, the object is referenced inside the exception, but the object participates in a cycle so it can't be destroyed via refcounting alone
var_dump(gc_collect_cycles()); // with $e gone, only the cycle is there. The collector collects the cycle but PHP explicitly supports 'reviving' an object via its destructor.

var_dump($hier);

Guess the output!
May depend on ini flags, running with -n on this PR certainly works.

@TimWolla TimWolla Sep 9, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤌

RETURN_THROWS();
}

/* Verify that both properties are positive, since the timelib_duration_ctor_static() takes unsigned. */
if (Z_LVAL_P(seconds) < 0 || Z_LVAL_P(nanoseconds) < 0) {
zend_throw_exception_ex(NULL, 0, "Invalid serialization data for %s object", ZSTR_VAL(duration->std.ce->name));
RETURN_THROWS();
}

int error = timelib_duration_ctor_static(&duration->duration, Z_LVAL_P(seconds), Z_LVAL_P(nanoseconds), Z_TYPE_P(negative) == IS_TRUE);
if (error != TIMELIB_ERROR_NO_ERROR) {
throw_timelib_error(error);
goto to_generic_error;
}

if (!duration_representable(&duration->duration)) {
throw_out_of_range_exception();
goto to_generic_error;
}

return;

to_generic_error:

/* Wrap the out of range error into a generic error. */
zend_throw_exception_ex(NULL, 0, "Invalid serialization data for %s object", ZSTR_VAL(duration->std.ce->name));
RETURN_THROWS();
}

PHP_METHOD(Time_Duration, fromSeconds)
{
zend_ulong seconds;
Expand Down
Loading