Skip to content

ext/standard: Free Windows DNS query results - #25035

Closed
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/aph-tl9d
Closed

iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:fix/aph-tl9d

Conversation

@iliaal

@iliaal iliaal commented Oct 29, 2026

Copy link
Copy Markdown
Member

Windows checkdnsrr() and dns_check_record() now free the result list after successful lookups. Source review found the leak, and repeated native Windows x64 lookups confirmed the correction.

Successful checkdnsrr() and dns_check_record() calls retain their native DNS
record lists. Release the list before returning.
Comment thread ext/standard/dns_win32.c
}
}

status = DnsQuery_A(hostname, type, DNS_QUERY_STANDARD, NULL, &pResult, NULL);

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.

Apparently, the pResult argument is optional. In that case, isn't a cleaner fix to pass NULL? Then you wouldn't need a DnsRecordListFree call I suppose

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Tried it. The status is the same either way, but with a NULL result pointer dnsapi still builds the record list and nothing frees it: 20k DnsQuery_A() calls on Windows x64 grow private bytes by ~6.5 MB per batch for an A lookup, at least as much as the unpatched code, while freeing the list stays flat. So the DnsRecordListFree() call is needed.

@iliaal
iliaal requested a review from ndossche October 29, 2026 15:25
@iliaal iliaal closed this in b175769 Oct 29, 2026
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.

2 participants