Skip to content
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

No ability to manage lifetime of ResolverProxy objects when browsing #13275

Closed
msandstedt opened this issue Dec 29, 2021 · 2 comments
Closed
Labels
stale Stale issue or PR v1.1

Comments

@msandstedt
Copy link
Contributor

msandstedt commented Dec 29, 2021

Problem

When the ResolverProxy object is initialized, a reference-counted ResolverDelegateProxy is allocated. However, it's not clear when (if ever) it would actually be safe to Shutdown the ResolverProxy, which releases the ResolverDelegateProxy, because our existing code may never be done calling back with browse results. This would be a use-after-free problem. And if this code is leaving the ResolverDelegateProxy with a reference count >= 1, then this a leak problem.

Proposed Solution

We need a method to un-register callbacks so we can cleanup these objects. A solution to #13227 may help.

bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Sep 22, 2022
Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Sep 22, 2022
Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275
bzbarsky-apple added a commit to bzbarsky-apple/connectedhomeip that referenced this issue Oct 3, 2022
Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275
andy31415 pushed a commit that referenced this issue Oct 4, 2022
* Add an API to stop a DNS-SD browse operation.

Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes #19194

May provide a way toward fixing
#13275

* Address review comments.

* Address more review comments.
@stale
Copy link

stale bot commented Dec 11, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Dec 11, 2022
@stale
Copy link

stale bot commented Dec 20, 2022

This stale issue has been automatically closed. Thank you for your contributions.

@stale stale bot closed this as completed Dec 20, 2022
rawadhilal88 pushed a commit to sharadb-amazon/connectedhomeip that referenced this issue Feb 22, 2023
…at fixes DNS-SD browsing

Add an API to stop a DNS-SD browse operation. (project-chip#22823)

* Add an API to stop a DNS-SD browse operation.

Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275

* Address review comments.

* Address more review comments.

[darwin] Use DNSServiceReconfirmRecord for A and AAAA records to miti… (project-chip#23067)

* [Dnssd] Add ReconfirmRecord method to verify address that appears to be out of date

* [SetUpCodePairer] Ask Dnssd to reconfirm discovered addresses if connecting to them ends with a CHIP_ERROR_TIMEOUT

Fix Logging When Trying to Log Nullptr To Strings (project-chip#23604)

This PR attempts to identify all cases where %s specifiers in the logging APIs
(ChipLogError(), ChipLogProgress(), ChipLogDetail()) don't have a guaranteed
non-null string parameter.

In all identified cases the issue is fixed using StringOrNullMarker() helper
method to guarantee it doesn't happen.

Use the "right" byte-swapping function for port in Darwin DnssdImpl. (project-chip#23894)

The incoming port is in host byte order and we are converting to network byte
order, so should use htons (which happens to do the same thing as ntohs, so no
behavior change).

Co-authored-by: Andrei Litvin <[email protected]>

Add a way for Resolver consumers to cancel operational resolve attempts. (project-chip#24010)

* Add a way for Resolver consumers to cancel operational resolve attempts.

Adds a way for consumers to notify Resolver when they no longer care
about an operational resolve, so a Resolver implementation can keep
track of how many consumers are interested and stop work as desired if
no one is interested.

Fixes project-chip#23881

* Address review comments.

* Address review comments.

Make sure we stop resolves triggered by a browse when the browse stops on Darwin. (project-chip#24733)

* Make sure we stop resolves triggered by a browse when the browse stops on Darwin.

Without this change, if there is a PTR record that matches whatever we are
browsing but no corresponding SRV record, we would end up leaking a resolve
forever.

Tested by modifying minimal mdns SrvResponder::AddAllResponses to no-op instead
of actually adding any responses, then trying to commission the device running
the modified minimal mdns.  Without this change, when the browse stops the
resolves it triggered keep going.  With this change, termination of the browse
also terminates the resolves.

Fixes project-chip#24074

* Also avoid leaking ResolveContext instances.

* Fix handling of multiple interfaces.

* Address review comment.

Improve discovery logging on Darwin. (project-chip#24846)

1) Use progress, not detail, logging, because detail logging is not actually
   persisted in system logs.
2) Add logging to a few functions that were missing it.

Remove the address type argument from ResolveNodeId. (project-chip#24006)

All consumers were passing kAny in practice, and some of the backends
(e.g. minimal mdns) had no capability to filter by type anyway.
rawadhilal88 pushed a commit to sharadb-amazon/connectedhomeip that referenced this issue Feb 22, 2023
* Add an API to stop a DNS-SD browse operation.

Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275

* Address review comments.

* Address more review comments.
cliffamzn pushed a commit to sharadb-amazon/connectedhomeip that referenced this issue Feb 23, 2023
* Add an API to stop a DNS-SD browse operation.

Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275

* Address review comments.

* Address more review comments.
sharadb-amazon pushed a commit to sharadb-amazon/connectedhomeip that referenced this issue Mar 9, 2023
* Add an API to stop a DNS-SD browse operation.

Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275

* Address review comments.

* Address more review comments.
sharadb-amazon pushed a commit to sharadb-amazon/connectedhomeip that referenced this issue Mar 17, 2023
* Add an API to stop a DNS-SD browse operation.

Most backends don't implement this yet. Darwin does, and no longer
stops Browse operations itself.

Fixes project-chip#19194

May provide a way toward fixing
project-chip#13275

* Address review comments.

* Address more review comments.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Stale issue or PR v1.1
Projects
None yet
Development

No branches or pull requests

3 participants