Jens Geyer created THRIFT-6260:
----------------------------------
Summary: PHP: the thrift_protocol serialize path dereferences spec
lookups without checking them
Key: THRIFT-6260
URL: https://issues.apache.org/jira/browse/THRIFT-6260
Project: Thrift
Issue Type: Bug
Components: PHP - Library
Reporter: Jens Geyer
In {{lib/php/src/ext/thrift_protocol/php_thrift_protocol.cpp}},
{{binary_serialize()}} takes sub-specs out of the field spec and dereferences
the result straight away:
{code:cpp}
// :922 (T_MAP)
HashTable* keyspec = Z_ARRVAL_P(zend_hash_str_find(fieldspec, "key",
sizeof("key")-1));
// :966 (T_SET)
HashTable* spec = Z_ARRVAL_P(zend_hash_str_find(fieldspec, "elem",
sizeof("elem")-1));
{code}
{{zend_hash_str_find()}} returns {{nullptr}} when the entry is absent, and
{{Z_ARRVAL_P}} dereferences it without a check, so a spec that is missing one
of these entries crashes the process rather than raising a PHP-level error.
The matching read path already guards every one of these lookups and raises
{{throw_tprotocolexception("no key type in spec", INVALID_DATA)}} --
{{binary_deserialize()}} at {{:728}}, {{:733}}, {{:758}}, {{:779}}, and
{{:610}} for {{class}}. The serialize path was not changed at the same time.
The spec is generated code rather than wire input, so this needs a hand-written
or out-of-date spec to reach. It is a robustness gap and an inconsistency
between the two directions, not a defect the remote end can drive.
h2. Suggested fix
Give the two sites above -- and any sibling in the same function -- the same
{{nullptr}} check and {{throw_tprotocolexception()}} the read path uses.
_Drafted with AI assistance (Claude Opus 5); reviewed and filed by Jens Geyer._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
