Skip to content

Fix GH-24748: socket_set_option() crash with declared object properties - #24749

Closed
lazerg wants to merge 6 commits into
php:PHP-8.4from
lazerg:fix/gh-24748-socket-option-object
Closed

lazerg wants to merge 6 commits into
php:PHP-8.4from
lazerg:fix/gh-24748-socket-option-object

Conversation

@lazerg

@lazerg lazerg commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Declared properties sit in the object's property table as indirect slots, and the SO_LINGER and SO_RCVTIMEO/SO_SNDTIMEO paths looked them up with zend_hash_str_find(), so zval_get_long() got an IS_INDIRECT zval and crashed. Using zend_hash_str_find_ind() follows the slot, and an uninitialized typed property now gives the usual "must have key" error.

Fixes #24748

@devnexen

Copy link
Copy Markdown
Member

can the following test case be added ?

$linger = new stdClass();
$linger->l_onoff = new stdClass();
$linger->l_linger = 5;
set_error_handler(function () use ($linger) {
    $tmp = (array) $linger;
    $linger->extra = 1;
    unset($tmp);
    return true;
});
socket_set_option(socket_create(AF_INET, SOCK_STREAM, SOL_TCP), SOL_SOCKET, SO_LINGER, $linger);

@lazerg

lazerg commented Oct 26, 2026

Copy link
Copy Markdown
Contributor Author

Added in 37088c0. That case still crashed, so the object's property table is now held with zend_get_properties_for() while the values are read.

Comment thread ext/sockets/sockets.c Outdated
RETURN_THROWS();
} else {
opt_ht = Z_OBJPROP_P(arg4);
opt_ht = opt_props = zend_get_properties_for(arg4, ZEND_PROP_PURPOSE_ARRAY_CAST);

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.

Suggested change
opt_ht = opt_props = zend_get_properties_for(arg4, ZEND_PROP_PURPOSE_ARRAY_CAST);
- opt_ht = opt_props = zend_get_properties_for(arg4, ZEND_PROP_PURPOSE_ARRAY_CAST);
+ opt_props = zend_get_properties_for(arg4, ZEND_PROP_PURPOSE_ARRAY_CAST);
+ opt_ht = opt_props ? opt_props : (HashTable *) &zend_empty_array;

@lazerg lazerg Oct 26, 2026 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Applied in 2f771be, for SO_RCVTIMEO/SO_SNDTIMEO as well.

@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.

Please address the lifetime issue noted inline.

Comment thread ext/sockets/sockets.c

zend_long val_lonoff = zval_get_long(l_onoff);
zend_long val_linger = zval_get_long(l_linger);
zend_release_properties(opt_props);

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.

zend_get_properties_for(..., ZEND_PROP_PURPOSE_ARRAY_CAST) can return a temporary table (for example, ArrayObject returns a duplicate). This release destroys that table, but the subsequent linger range-error formatter still passes l_linger to %s, causing a use-after-free read.

The incorrect format argument predates this PR; freeing its backing table before formatting introduces the lifetime defect. Please pass l_linger_key instead of l_linger to zend_argument_value_error() and add regression coverage for this validation path with a temporary property table.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 312d69e. The error now uses l_linger_key, and gh24748.phpt covers it with an out-of-range ArrayObject.

@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.

Thanks for fix, i'll merge once ci/cd passes

@iliaal iliaal closed this in 7adcf5a Oct 27, 2026
@Girgias

Girgias commented Oct 27, 2026

Copy link
Copy Markdown
Member

We normally just removed support for code that crashes as clearly nobody used it.

@iliaal

iliaal commented Oct 27, 2026

Copy link
Copy Markdown
Member

We normally just removed support for code that crashes as clearly nobody used it.

I'll +1 the RFC to deprecate/remove it if you write it ;-)

@Girgias

Girgias commented Oct 27, 2026

Copy link
Copy Markdown
Member

It crashes, why are you trying to fix code that crashes in a way that will get deprecated?

@iliaal

iliaal commented Oct 27, 2026

Copy link
Copy Markdown
Member

The RFC line was tongue-in-cheek. More to the point, this isn't code that always crashed: through 8.3.15 and 8.4.2 the value went through convert_to_array(), so any object worked, declared properties included. 8a649a8 switched to reading Z_OBJPROP_P() directly, and the crash is a regression from it. #24800 would make (object)['sec' => 1, 'usec' => 0], which works in every release so far, throw a TypeError in a patch release. Dropping object support on master is fine by me; stable branches should keep the fix. #24800 also passes l_linger to %s in the range error again.

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.

4 participants