Skip to content

Fix WeakMap object reference offset causing TypeError#8995

Merged
arnaud-lb merged 2 commits into
php:PHP-8.0from
Nevay:fix/weakmap-object-reference
Jul 15, 2022
Merged

Fix WeakMap object reference offset causing TypeError#8995
arnaud-lb merged 2 commits into
php:PHP-8.0from
Nevay:fix/weakmap-object-reference

Conversation

@Nevay

@Nevay Nevay commented Jul 13, 2022

Copy link
Copy Markdown
Contributor

Fix TypeError when using object reference as WeakMap offset.

$a = new stdClass();
$b = &$a;
$map = new WeakMap();
$map[$a] = true;
Fatal error: Uncaught TypeError: WeakMap key must be an object

iluuu1994
iluuu1994 reviewed Jul 13, 2022
View reviewed changes
Comment thread Zend/zend_weakrefs.c Outdated
cmb69
cmb69 approved these changes Jul 13, 2022
View reviewed changes

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

Thank you! That looks about right, although I don't quite understand the use case (why referencing an object?)

@Nevay

Nevay commented Jul 13, 2022

Copy link
Copy Markdown
Contributor Author

I stumbled across this while using a by-ref parameter which is stored in a WeakMap afterwards (ref-1, ref-2):

function doStuff(?object &$obj = null): void {
    $obj = new stdClass();
}
doStuff($obj);
$map = new WeakMap();
$map[$obj] = null;

@cmb69

cmb69 commented Jul 13, 2022

Copy link
Copy Markdown
Member

Ah, yeah, that makes sense (although I wish PHP had out parameters for such purposes).

@arnaud-lb
arnaud-lb merged commit ede92a8 into php:PHP-8.0 Jul 15, 2022
@arnaud-lb

arnaud-lb commented Jul 15, 2022

Copy link
Copy Markdown
Member

Thank you !

arnaud-lb added a commit that referenced this pull request Jul 15, 2022
@arnaud-lb
Merge branch 'PHP-8.1'
4552941 
* PHP-8.1:
  [ci skip] NEWS
  Fix `WeakMap` object reference offset causing `TypeError` (#8995)
@brettmc brettmc mentioned this pull request Feb 29, 2024
Merged
2 tasks
@agibson-godaddy agibson-godaddy mentioned this pull request Mar 31, 2026
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iluuu1994 iluuu1994 iluuu1994 left review comments
@cmb69 cmb69 cmb69 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Nevay @cmb69 @arnaud-lb @iluuu1994