-
Notifications
You must be signed in to change notification settings - Fork 573
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
Race condition on logger->host on iOS/macOS #265
Comments
Thanks for the detailed write-up! I think that at this stage, the best solution would be to shield access to That would be a less invasive change than splitting the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 inLoggerSetViewerHost
while the worker thread is using it.The following code causes the application to crash (about 50% of the time) in
LoggerStartReachabilityChecking
,CFStreamCreatePairWithSocketToHost
, orCFWriteStreamOpen
.You're probably asking yourself:
🙋♂️
I've observed this crash in production code where the
LoggerSetViewerHost
function is called before each call toLogMessage
(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
andpthread_mutex_unlock
could be added wheneverlogger->host
is used. I think this would be very error-prone.The text was updated successfully, but these errors were encountered: