Skip to content

[SR-5761] corelibs-foundation: fix concurrency/fd ownership in MultiHandle.swift #3813

@weissi

Description

@weissi
Previous ID SR-5761
Radar None
Original Reporter @weissi
Type Bug
Additional Detail from JIRA
Votes 0
Component/s Foundation
Labels Bug
Assignee None
Priority Medium

md5: 7e7084911c5c1527e92b7224cdfe2621

Issue Description:

libcurl requires you to call all call related to one handle on the same thread and MultiHandle.swift violates that in a few places, eg (incomplete list):

  • calls CFURLSessionMultiHandleRemoveHandle handles in deinit but we can't know the thread of where that's called

  • calls some CFURLSessionMultiHandle* functions on the initial callers thread and others on self.queue

  • calls others within commandQueue.async (fixed by https://github.com/apple/swift-corelibs-foundation/pull/1188/files )

On top of that, the ownership of the file descriptor that we get from libcurl in this bit

        try! CFURLSession_multi_setopt_sf(rawHandle, CFURLSessionMultiOptionSOCKETFUNCTION) { (easyHandle: CFURLSessionEasyHandle, socket: CFURLSession_socket_t, what: Int32, userdata: UnsafeMutableRawPointer?, socketptr: UnsafeMutableRawPointer?) -> Int32 in
            guard let handle = URLSession._MultiHandle.from(callbackUserData: userdata) else { fatalError() }
            return handle.register(socket: socket, for: easyHandle, what: what, socketSourcePtr: socketptr)
            }.asError()

isn't correctly maintained. In deinit (which is wrong in itself) we just do this:

        deinit {
            // C.f.: <https://curl.haxx.se/libcurl/c/curl_multi_cleanup.html>
            easyHandles.forEach {
                try! CFURLSessionMultiHandleRemoveHandle(rawHandle, $0.rawHandle).asError()
            }
            try! CFURLSessionMultiHandleDeinit(rawHandle).asError()
        }

which will probably close(2) the file descriptor. That is only legal however if all the DisatchSources have run their cancellation handler.

So for any fd/DispatchSource we have, we'll need something like this

let fd = /* fd from curl */
let fdGroup = DispatchGroup()
fdGroup.notify(queue: curlQueue) {
  CFURLSessionMultiHandleRemoveHandle(...)
}

and for every source we create, we need this

fdGroup.enter()
let src = DispatchSource.makeReadSource(fd: fd, ...)
src.setCancellationHandler {
  fdGroup.leave()
}

that'll make sure that every existing DispatchSource for a given file descriptor has fully deregistered before we allow curl to close it.

I haven't worked out all the details but there's quite a bit of work. I can do this after my holiday, this ticket to remind me.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions