Skip to content

ext/ldap: fix crash in ldap_exop_sync() when $response_data is omitted - #23339

Open
lacatoire wants to merge 1 commit into
php:PHP-8.4from
lacatoire:fix/ldap-exop-sync-null-response-data
Open

lacatoire wants to merge 1 commit into
php:PHP-8.4from
lacatoire:fix/ldap-exop-sync-null-response-data

Conversation

@lacatoire

Copy link
Copy Markdown
Member

ldap_exop_sync() passes force_sync = true to php_ldap_exop(), so the block that writes back $response_data is always entered regardless of whether the argument was supplied. When it is omitted, retdata is NULL, and both ZEND_TRY_ASSIGN_REF_STRINGL and ZEND_TRY_ASSIGN_REF_EMPTY_STRING dereference it — ZEND_ASSERT(Z_ISREF_P(zv)) in debug builds, a NULL dereference crash in release.

ldap_exop() is not affected: there retdata != NULL is what selects the branch in the first place.

The fix mirrors the existing if (retoid) guard a few lines above. ldap_memfree() is still called unconditionally so there is no memory leak.

ldap_exop_sync($ld, LDAP_EXOP_WHO_AM_I);  // segfault before, bool(true) after

Reproduced on PHP 8.4.22 (exit code 139). Present since ldap_exop_sync() was introduced in PHP 8.3.0.

@iliaal

iliaal commented Oct 28, 2026

Copy link
Copy Markdown
Member

This fixes GH-25005. Please retarget the base to PHP-8.4 and rebase onto that branch. The unguarded retdata assignments are unchanged there.

@iliaal
iliaal self-requested a review October 28, 2026 17:21
ldap_exop_sync() always takes the synchronous branch of php_ldap_exop(),
which assigns $response_data unconditionally. When the argument is not
passed, retdata is NULL and ZEND_TRY_ASSIGN_REF_STRINGL()/
ZEND_TRY_ASSIGN_REF_EMPTY_STRING() dereference it, so any successful
ldap_exop_sync($ldap, $oid) call segfaults. Guard the assignment the way
the $response_oid one already is.
@lacatoire
lacatoire force-pushed the fix/ldap-exop-sync-null-response-data branch from fce4c8f to 1fd085c Compare October 29, 2026 10:58
@lacatoire
lacatoire changed the base branch from master to PHP-8.4 October 29, 2026 10:58

@iliaal iliaal left a comment

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.

LGTM, thanks for rebasing to 8.4

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.

ldap_exop_sync() crashes when the optional $response_data argument is omitted

2 participants