Skip to content

ext/date: guard property-table reads in php_date_interval_initialize_from_hash()#22822

Open
brzuchal wants to merge 1 commit into
php:masterfrom
brzuchal:fix-dateinterval-property-table-reads
Open

ext/date: guard property-table reads in php_date_interval_initialize_from_hash()#22822
brzuchal wants to merge 1 commit into
php:masterfrom
brzuchal:fix-dateinterval-property-table-reads

Conversation

@brzuchal

Copy link
Copy Markdown
Contributor

Problem

php_date_interval_initialize_from_hash() reads the interval fields out of a HashTable
that, for an uninitialized DateInterval, is the object's raw property table:
date_object_get_properties_interval() returns zend_std_get_properties() unchanged when
!intervalobj->initialized.

A raw property table stores declared properties as IS_INDIRECT slots. Two of the fields are
read open-coded without a type guard and hand that zval straight to zval_get_double() /
zval_get_long(), which have no case IS_INDIRECT: and fall into ZEND_UNREACHABLE().

Reproducer

<?php
class B extends DateInterval { public float $f = 1.5; }

$o = (new ReflectionClass('B'))->newInstanceWithoutConstructor();
$o->__wakeup();
var_dump($o->f);

Debug build:

Assertion failed: (0), function zval_get_double_func, file zend_operators.c, line 1056.

Release build: no diagnostic at all — the declared value is silently discarded.

newInstanceWithoutConstructor() is what leaves initialized false. unserialize() is not a
route here, because DateInterval defines __unserialize() and that takes precedence over
__wakeup().

Fix

The PHP_DATE_INTERVAL_READ_PROPERTY() family already accounts for this — all three variants
(plain, _I64, _DAYS) guard with Z_TYPE_P(z_arg) <= IS_STRING, which excludes
IS_INDIRECT (12). Eighteen fields are read through them and are safe. Only "f" and
"civil_or_wall" are read open-coded, and they lack the guard; this applies the same guard to
both.

ext/zlib solves the same class of problem at the call site with ZVAL_DEINDIRECT(); the
comment there notes that the |H ZPP specifier may leave HashTable entries wrapped in
IS_INDIRECT.

Behaviour

On an uninitialized instance, a DateInterval subclass declaring $f or $civil_or_wall now
has those fields ignored deterministically — exactly as the eighteen sibling fields already
are — instead of hitting undefined behaviour. Nothing else changes: a plain DateInterval,
dynamic properties, and the normal serialize/unserialize round-trip are unaffected.

Tests

New ext/date/tests/date_interval_wakeup_declared_property.phpt covers the
declared-initialized, declared-uninitialized and civil_or_wall cases, plus a
dynamic-property control (still read correctly) and a serialize/unserialize round-trip.

ext/date/tests: 680/680 pass, debug build.

…from_hash()

php_date_interval_initialize_from_hash() reads the interval fields out of a
HashTable which, for an uninitialized DateInterval, is the object's raw
property table: date_object_get_properties_interval() returns
zend_std_get_properties() unchanged when !intervalobj->initialized.

A raw property table stores declared properties as IS_INDIRECT slots. The
PHP_DATE_INTERVAL_READ_PROPERTY() family already accounts for this with a
Z_TYPE_P(z_arg) <= IS_STRING guard, so the eighteen fields read through it
are safe. The two fields read open-coded, "f" and "civil_or_wall", have no
such guard and pass the IS_INDIRECT zval straight to zval_get_double() and
zval_get_long(), which do not accept it and fall into ZEND_UNREACHABLE().

  class B extends DateInterval { public float $f = 1.5; }
  $o = (new ReflectionClass('B'))->newInstanceWithoutConstructor();
  $o->__wakeup();

In a debug build this aborts on an assertion in zval_get_double_func(). In a
release build it is undefined behaviour; as observed the declared value is
silently discarded rather than diagnosed.

ext/zlib solves the same class of problem at the call site with
ZVAL_DEINDIRECT(), whose comment in zlib.c notes that the |H ZPP specifier
may leave HashTable entries wrapped in IS_INDIRECT.

Apply the guard the sibling reads already use, and add a regression test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant