Skip to content
66 changes: 42 additions & 24 deletions Sources/AsyncDNSResolver/dnssd/DNSResolver_dnssd.swift
Original file line number Diff line number Diff line change
Expand Up @@ -110,20 +110,7 @@ struct DNSSD {
let recordStream = AsyncThrowingStream<ReplyHandler.Record, Error> { continuation in
let handler = QueryReplyHandler(handler: replyHandler, continuation)

// Wrap `handler` into a pointer so we can pass it to DNSServiceQueryRecord
let handlerPointer = UnsafeMutableRawPointer.allocate(
byteCount: MemoryLayout<QueryReplyHandler>.stride,
alignment: MemoryLayout<QueryReplyHandler>.alignment
)

handlerPointer.initializeMemory(as: QueryReplyHandler.self, repeating: handler, count: 1)

// The handler might be called multiple times so don't deallocate inside `callback`
defer {
let pointer = handlerPointer.assumingMemoryBound(to: QueryReplyHandler.self)
pointer.deinitialize(count: 1)
pointer.deallocate()
}
let query = Query(handler: handler)

// This is called once per record received
let callback: DNSServiceQueryRecordReply = { _, _, _, errorCode, _, _, _, rdlen, rdata, _, context in
Expand All @@ -138,32 +125,42 @@ struct DNSSD {
handler.handleRecord(errorCode: errorCode, data: rdata, length: rdlen)
}

let serviceRefPtr = UnsafeMutablePointer<DNSServiceRef?>.allocate(capacity: 1)
defer { serviceRefPtr.deallocate() }

// Run the query
let _code = DNSServiceQueryRecord(
serviceRefPtr,
query.serviceRefPtr,
kDNSServiceFlagsTimeout,
0,
name,
UInt16(type.kDNSServiceType),
UInt16(kDNSServiceClass_IN),
callback,
handlerPointer
query.replyHandlerPointer
)

// Check if query completed successfully
guard _code == kDNSServiceErr_NoError else {
return continuation.finish(throwing: AsyncDNSResolver.Error(dnssdCode: _code))
}

// Read reply from the socket (blocking) then call reply handler
DNSServiceProcessResult(serviceRefPtr.pointee)
DNSServiceRefDeallocate(serviceRefPtr.pointee)
let serviceSockFD = DNSServiceRefSockFD(query.serviceRefPtr.pointee)
guard serviceSockFD != -1 else {
return continuation.finish(throwing: AsyncDNSResolver.Error(code: .internalError, message: "Failed to access the DNSSD service socket"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't we leaking the serviceRefPointer and replyHandlerPointer in this branch?

Copy link
Author

Choose a reason for hiding this comment

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

I changed the code so that the deallocates are also called in this branch and the other error branch just above.

}

let readSource = DispatchSource.makeReadSource(fileDescriptor: serviceSockFD)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be passing our own queue here rather than using the default?

Copy link
Member

Choose a reason for hiding this comment

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

isn't this file descriptor leaked?

Copy link
Author

Choose a reason for hiding this comment

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

I close it now in the cancellation handler

Copy link
Member

Choose a reason for hiding this comment

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

you'll need .setCancelHandler here and manage the resources from there.

DispatchSources that use file descriptors and don't use setCancelHandler are guaranteed to be incorrect. Also see https://developer.apple.com/documentation/dispatch/1385648-dispatch_source_set_cancel_handl?language=objc#discussion

Copy link
Author

Choose a reason for hiding this comment

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

Yes, thanks for the feedback! I implemented it in the latest commit

readSource.setEventHandler {
// Read reply from the socket (blocking) then call reply handler
DNSServiceProcessResult(query.serviceRefPtr.pointee)

// Streaming done
continuation.finish()
// Streaming done
continuation.finish()
Copy link
Member

Choose a reason for hiding this comment

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

this should cancel the read source

Copy link
Author

Choose a reason for hiding this comment

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

Applied in latest commit

}
readSource.resume()

continuation.onTermination = { _ in
readSource.cancel()
DNSServiceRefDeallocate(query.serviceRefPtr.pointee)
Copy link
Member

Choose a reason for hiding this comment

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

security relevant use after free bug here, you could be concurrently deallocating this as well as using it in the event handler

Copy link
Author

Choose a reason for hiding this comment

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

The deallocation is now done in the dispatch source cancel handler so that should no longer happen

}
}

// Build reply using records received
Expand All @@ -179,6 +176,27 @@ struct DNSSD {

@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
extension DNSSD {
// Class used to manage both the handler and the serviceRef pointers, so that they are deallocated when we are done using them
class Query {
let serviceRefPtr = UnsafeMutablePointer<DNSServiceRef?>.allocate(capacity: 1)

let replyHandlerPointer = UnsafeMutableRawPointer.allocate(
byteCount: MemoryLayout<QueryReplyHandler>.stride,
alignment: MemoryLayout<QueryReplyHandler>.alignment
)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a difference in initializing serviceRefPtr and replyHandlerPointer here vs. inside the initializer? Does anyone have any preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Slight preference towards doing it in the init so the allocate and initializeMemory calls are next to each other.

init(handler: QueryReplyHandler) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: add newline before

// Wrap 'handler' into a pointer so we can pass it to DNSServiceQueryRecord
self.replyHandlerPointer.initializeMemory(as: QueryReplyHandler.self, repeating: handler, count: 1)
}

deinit {
serviceRefPtr.deallocate()
let pointer = replyHandlerPointer.assumingMemoryBound(to: QueryReplyHandler.self)
pointer.deinitialize(count: 1)
pointer.deallocate()
}
}

class QueryReplyHandler {
private let _handleRecord: (DNSServiceErrorType, UnsafeRawPointer?, UInt16) -> Void

Expand Down