Skip to content

Commit b98f746

Browse files
committed
php#53: Fix ref_count management for async event callbacks
This commit fixes a critical memory management issue in the async event callback system where callbacks were created with ref_count=1 but add methods didn't increment the counter, leading to premature deallocation. Changes: - Initialize all callbacks with ref_count=0 instead of 1 - Add ref_count increment in callback registration methods - Remove incorrect ref_count validation in zend_async_resume_when Modified files: * Zend/zend_async_API.c: Fix zend_async_coroutine_callback_new and waker_trigger_add_callback * Zend/zend_async_API.h: Fix zend_async_callbacks_push * main/network_async.c: Fix 5 direct callback initializations for poll_callback_t, select_callback_t, async_stream_callback_t, and dns_callback_t structures This ensures proper lifetime management for all async callback types including socket, DNS, stream, and scope callbacks.
1 parent fe716b9 commit b98f746

File tree

3 files changed

+24
-20
lines changed

3 files changed

+24
-20
lines changed

Zend/zend_async_API.c

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -451,7 +451,7 @@ ZEND_API zend_coroutine_event_callback_t *zend_async_coroutine_callback_new(
451451
zend_coroutine_event_callback_t *coroutine_callback
452452
= ecalloc(1, size != 0 ? size : sizeof(zend_coroutine_event_callback_t));
453453

454-
coroutine_callback->base.ref_count = 1;
454+
coroutine_callback->base.ref_count = 0;
455455
coroutine_callback->base.callback = callback;
456456
coroutine_callback->coroutine = coroutine;
457457
coroutine_callback->base.dispose = coroutine_event_callback_dispose;
@@ -773,7 +773,7 @@ ZEND_API void zend_async_resume_when(zend_coroutine_t *coroutine, zend_async_eve
773773
{
774774
zend_exception_save();
775775

776-
bool locally_allocated_callback = false;
776+
bool callback_should_dispose = false;
777777

778778
if (UNEXPECTED(ZEND_ASYNC_EVENT_IS_CLOSED(event))) {
779779
zend_throw_error(NULL, "The event cannot be used after it has been terminated");
@@ -794,11 +794,14 @@ ZEND_API void zend_async_resume_when(zend_coroutine_t *coroutine, zend_async_eve
794794

795795
if (event_callback == NULL) {
796796
event_callback = emalloc(sizeof(zend_coroutine_event_callback_t));
797-
event_callback->base.ref_count = 1;
797+
event_callback->base.ref_count = 0;
798798
event_callback->base.callback = callback;
799799
event_callback->base.dispose = coroutine_event_callback_dispose;
800800
event_callback->event = event;
801-
locally_allocated_callback = true;
801+
callback_should_dispose = true;
802+
} else if (event_callback->base.ref_count == 0) {
803+
// Refcount is 0 means someone is transfer of ownership
804+
callback_should_dispose = true;
802805
}
803806

804807
// Set up the default dispose function if not set
@@ -811,17 +814,14 @@ ZEND_API void zend_async_resume_when(zend_coroutine_t *coroutine, zend_async_eve
811814
event_callback->event = event;
812815
}
813816

814-
if (event_callback->base.ref_count == 0) {
815-
event_callback->base.ref_count = 1;
816-
}
817-
818-
ZEND_ASSERT(event_callback->base.ref_count > 0 && "Callback ref_count must be greater than 0.");
817+
ZEND_ASSERT(event_callback->base.ref_count >= 0 && "Callback ref_count must be non-negative.");
819818

820819
event_callback->coroutine = coroutine;
821820
event->add_callback(event, &event_callback->base);
822821

823822
if (UNEXPECTED(EG(exception) != NULL)) {
824-
if (locally_allocated_callback) {
823+
if (callback_should_dispose) {
824+
event_callback->base.ref_count = 1;
825825
event_callback->base.dispose(&event_callback->base, event);
826826
}
827827

@@ -860,7 +860,8 @@ ZEND_API void zend_async_resume_when(zend_coroutine_t *coroutine, zend_async_eve
860860
event_callback->coroutine = NULL;
861861
event->del_callback(event, &event_callback->base);
862862

863-
if (locally_allocated_callback) {
863+
if (callback_should_dispose) {
864+
event_callback->base.ref_count = 1;
864865
event_callback->base.dispose(&event_callback->base, event);
865866
}
866867

Zend/zend_async_API.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -674,6 +674,7 @@ static zend_always_inline void zend_async_callbacks_push(
674674
vector->data, vector->capacity, sizeof(zend_async_event_callback_t *), 0);
675675
}
676676

677+
callback->ref_count++;
677678
vector->data[vector->length++] = callback;
678679
}
679680

main/network_async.c

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,7 @@ ZEND_API int php_poll2_async(php_pollfd *ufds, unsigned int nfds, int timeout)
529529
// so it can update the revents field when the event occurs.
530530
poll_callback_t * callback = ecalloc(1, sizeof(poll_callback_t));
531531
callback->callback.coroutine = coroutine;
532-
callback->callback.base.ref_count = 1;
532+
callback->callback.base.ref_count = 0;
533533
callback->callback.base.callback = poll_callback_resolve;
534534
callback->ufd = &ufds[i];
535535

@@ -746,7 +746,7 @@ ZEND_API int php_select_async(php_socket_t max_fd, fd_set *rfds, fd_set *wfds, f
746746

747747
select_callback_t * callback = ecalloc(1, sizeof(select_callback_t));
748748
callback->callback.coroutine = coroutine;
749-
callback->callback.base.ref_count = 1;
749+
callback->callback.base.ref_count = 0;
750750
callback->callback.base.callback = select_callback_resolve;
751751
callback->fd = i;
752752
callback->rfds = &aread;
@@ -952,7 +952,7 @@ static zend_always_inline bool process_stream_array(
952952

953953
async_stream_callback_t *callback = ecalloc(1, sizeof(async_stream_callback_t));
954954
callback->callback.coroutine = coroutine;
955-
callback->callback.base.ref_count = 1;
955+
callback->callback.base.ref_count = 0;
956956
callback->callback.base.callback = async_stream_callback_resolve;
957957
callback->stream = stream;
958958
callback->event = poll_event;
@@ -1419,6 +1419,8 @@ static void dns_nameinfo_callback_resolve(
14191419

14201420
if (dns_callback->hostname_result != NULL) {
14211421
*(dns_callback->hostname_result) = dns_event->hostname;
1422+
// Should be free by the caller using zend_string_release (php_network_gethostbyaddr_async)
1423+
zend_string_addref(dns_event->hostname);
14221424
}
14231425

14241426
ZVAL_TRUE(&coroutine->waker->result);
@@ -1456,7 +1458,7 @@ ZEND_API int php_network_getaddrinfo_async(const char *node, const char *service
14561458

14571459
dns_callback_t *callback = ecalloc(1, sizeof(dns_callback_t));
14581460
callback->callback.coroutine = coroutine;
1459-
callback->callback.base.ref_count = 1;
1461+
callback->callback.base.ref_count = 0;
14601462
callback->callback.base.callback = dns_callback_resolve;
14611463
callback->result = res;
14621464

@@ -1639,7 +1641,7 @@ ZEND_API zend_string* php_network_gethostbyaddr_async(const char *ip)
16391641
zend_string *hostname_result = NULL;
16401642
dns_callback_t *callback = ecalloc(1, sizeof(dns_callback_t));
16411643
callback->callback.coroutine = coroutine;
1642-
callback->callback.base.ref_count = 1;
1644+
callback->callback.base.ref_count = 0;
16431645
callback->callback.base.callback = dns_nameinfo_callback_resolve;
16441646
callback->hostname_result = &hostname_result;
16451647

@@ -1659,16 +1661,16 @@ ZEND_API zend_string* php_network_gethostbyaddr_async(const char *ip)
16591661

16601662
IF_EXCEPTION_GOTO_ERROR;
16611663

1662-
if (hostname_result != NULL) {
1663-
zend_string_addref(hostname_result);
1664-
}
1665-
16661664
if (Z_TYPE(coroutine->waker->result) == IS_TRUE) {
16671665
zend_async_waker_clean(coroutine);
16681666
return hostname_result;
16691667
}
16701668

16711669
error:
1670+
if (hostname_result != NULL) {
1671+
zend_string_release(hostname_result);
1672+
}
1673+
16721674
zend_async_waker_clean(coroutine);
16731675
dns_handle_exception_and_errno();
16741676
return NULL;

0 commit comments

Comments
 (0)