-
Notifications
You must be signed in to change notification settings - Fork 7.9k
GH-18572: infinite stack recursion in fallback object comparison. #18577
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.
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
Changes from 4 commits
d8862f9
2c540ab
6fda779
5b6340b
e13067a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
--TEST-- | ||
GH-18519: Nested object comparison leading to stack overflow | ||
--SKIPIF-- | ||
<?php | ||
if (getenv('SKIP_ASAN')) die('skip as it fatally crash'); | ||
?> | ||
--FILE-- | ||
<?php | ||
|
||
error_reporting(E_ALL & ~E_DEPRECATED); | ||
|
||
class Node { | ||
public $next; | ||
// forcing dynamic property creation is key | ||
} | ||
|
||
$first = new Node(); | ||
$first->previous = $first; | ||
$first->next = $first; | ||
|
||
$cur = $first; | ||
|
||
for ($i = 0; $i < 50000; $i++) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The test "fail" even for i=0 (no loop at all) - https://3v4l.org/gkKqd. The comparison probably cannot find cycles correctly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it does trigger the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have simplified the code and opened #18585. I belive the recursion in weak comparasion should be handled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
$new = new Node(); | ||
$new->previous = $cur; | ||
$cur->next = $new; | ||
$new->next = $first; | ||
$first->previous = $new; | ||
$cur = $new; | ||
} | ||
|
||
try { | ||
// Force comparison manually to trigger zend_hash_compare | ||
$first == $cur; | ||
} catch(Error $e) { | ||
echo $e->getMessage(). PHP_EOL; | ||
} | ||
?> | ||
--EXPECTREGEX-- | ||
(Object compare - stack limit reached|Fatal error: Nesting level too deep - recursive dependency?.+) |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -42,6 +42,15 @@ | |||||||
#define IN_UNSET ZEND_GUARD_PROPERTY_UNSET | ||||||||
#define IN_ISSET ZEND_GUARD_PROPERTY_ISSET | ||||||||
|
||||||||
static zend_always_inline bool zend_objects_check_stack_limit(void) | ||||||||
{ | ||||||||
#ifdef ZEND_CHECK_STACK_LIMIT | ||||||||
return zend_call_stack_overflowed(EG(stack_limit)); | ||||||||
#else | ||||||||
return false; | ||||||||
#endif | ||||||||
} | ||||||||
|
||||||||
/* | ||||||||
__X accessors explanation: | ||||||||
|
||||||||
|
@@ -1714,6 +1723,11 @@ ZEND_API int zend_std_compare_objects(zval *o1, zval *o2) /* {{{ */ | |||||||
{ | ||||||||
zend_object *zobj1, *zobj2; | ||||||||
|
||||||||
if (zend_objects_check_stack_limit()) { | ||||||||
zend_throw_error(NULL, "Object compare - stack limit reached"); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit:
Suggested change
To get a similar message as in Line 114 in 05618e7
|
||||||||
return ZEND_UNCOMPARABLE; | ||||||||
} | ||||||||
|
||||||||
if (Z_TYPE_P(o1) != Z_TYPE_P(o2)) { | ||||||||
/* Object and non-object */ | ||||||||
zval *object; | ||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Add
#[AllowDynamicProperties]
on the class to avoid the warning