Race condition on logger->host on iOS/macOS

[baarde]

Environment

NSLogger 1.9.0 for iOS

Problem

I've noticed the worker thread does not lock when reading the value of logger->host. This mean we can release the previous value in LoggerSetViewerHost while the worker thread is using it.

The following code causes the application to crash (about 50% of the time) in LoggerStartReachabilityChecking, CFStreamCreatePairWithSocketToHost, or CFWriteStreamOpen.

LoggerSetViewerHost(nil, "192.168.1.25" as CFString, 19399)
LoggerStart(nil)
for _ in 0...999999 {
    LoggerSetViewerHost(nil, "192.168.1.25" as CFString, 19399)
}

You're probably asking yourself:

But who would do such a stupid thing?!

🙋‍♂️

I've observed this crash in production code where the LoggerSetViewerHost function is called before each call to LogMessage (the viewer host is configured on the server and may change during the application lifetime).

Workaround

The issue can be avoided by creating a different Logger when the viewer host changes.

Fixing the issue

I'd be happy to work on this issue I you think it's worth solving (the workaround works great for me but maybe other developers are/will be affected by this issue).

Proposal

The Logger struct could be partitioned with a protected and an unprotected part. The protected part would be accessible only to the worker thread. The unprotected part would be accessible from everywhere and require locking. When a change is signalled to the worker thread, the new value is copied from the unprotected part to the protected part.

Alternatives

The LoggerSetViewerHost function could be modified to be a NO-OP when the new host and port are equal to the previous ones. This is trivial and would prevent most crashes (how often does the host actually change?) but not all of them.

Calls to pthread_mutex_lock and pthread_mutex_unlock could be added whenever logger->host is used. I think this would be very error-prone.

[fpillet]

Thanks for the detailed write-up! I think that at this stage, the best solution would be to shield access to logger->host with one of the mutexes. It's not so error-prone, we can write a pair of functions that perform the protected read and write.

That would be a less invasive change than splitting the Logger struct, imho