Skip to content

Conversation

arnaud-lb
Copy link
Member

@arnaud-lb arnaud-lb commented Aug 21, 2025

Possible fix for GH-19543.

Since cbf67e4, the GC needs to find all WeakMaps referencing a weakly referenced object. Doing so, it treats all ZEND_WEAKREF_TAG_MAP as WeakMap instances.

However, a ZEND_WEAKREF_TAG_MAP reference may be a bare HashTable when zend_weakrefs_hash_add() is used.

Introduce a new tag, ZEND_WEAKREF_TAG_BARE_HT, and use this tag when weakly referencing an object from a bare HashTable. Ignore such references in GC.

cc @bwoebi @TimWolla

Edit: should probably target 8.3.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Would it make sense to add explicit handling to zend_weakmap_get_*_gc()? Something like:

	else if (tag == ZEND_WEAKREF_TAG_BARE_HT) {
		/* Bare HashTables are intentionally ignored, since they are intended for internal usage by extensions and might be allocated in globals. */
	}

Basically to serve as documentation for the future reader.

@TimWolla
Copy link
Member

For proper attribution:

The test case is Co-authored-by: Tim Düsterhus <tim@tideways-gmbh.com> (different email address, this was done on company time).

@TimWolla
Copy link
Member

Edit: should probably target 8.3.

Given that this does not touch any header files, it should be safe to fix for 8.3. Nevertheless extensions would need to include workarounds either way, since they can't necessarily guarantee they are running on a fixed version of PHP.

arnaud-lb and others added 3 commits August 21, 2025 20:36
Co-authored-by: Tim Düsterhus <tim@tideways-gmbh.com>
@arnaud-lb arnaud-lb changed the base branch from master to PHP-8.3 August 21, 2025 18:40
@arnaud-lb arnaud-lb marked this pull request as ready for review August 21, 2025 19:37
Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

Soft LGTM. I'll double-check this against my actual use-case (incl. my work-around) to verify that this doesn't break anything before approving properly.

Copy link
Member

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

For my use case I can confirm that:

  • 8.3 without this PR crashes.
  • 8.3 with this PR works.
  • 8.3 without this PR and my workaround works.
  • 8.3 with this PR and my workaround works.

Thus this PR improves the situation without the workaround and doesn't break the workaround.

Copy link
Member

@bwoebi bwoebi left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you!

@arnaud-lb arnaud-lb closed this in d74901a Aug 22, 2025
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.

3 participants