Thread sanitizer finds issues #237
Comments
|
I can recreate this using the https://github.com/IBM-Swift-Sunset/Kitura-Chat-Server sample, and also with another simple project that does not use WebSockets. Most of the reported races involve flags or state relating to the lifespan of a connection. After a new connection is accepted, we invoke Another race apparently exists when first handling an incoming connection: when the IncomingSocketManager creates the IncomingSocketHandler, the DispatchSourceRead is resumed from within the initializer. The IncomingSocketManager then reads Regardless of whether either race is likely to manifest in a bug in practice, their presence could make it harder to use the thread sanitizer tool to find other more pressing problems. I'll look into what's involved in fixing these. |
|
This may have been fixed by #284 - this required locking around the socketHandlers array to prevent concurrent access during mutation with Swift 5. I'll try to confirm in the next week or so. |
|
These are not fixed, I was confusing two different issues in my mind. Although TSan would have flagged up the socketHandlers concurrent access if it were available on Linux. A simple fix for these data races is to guard each of the state variables that could be accessed by two threads with a read/write lock, similar to the one I used in #284. However, a first pass at doing this tanked our Hello World performance by almost 50%. I'll continue experimenting to see if I can find an implementation that has a performance penalty that we can stomach. |
|
These issues can manifest as arbitrary memory unsafety and therefore security vulnerabilities. I would very very much recommend fixing them asap. Thread unsafety in Swift is especially troublesome because it breaks copy on write and make a usually memory safe language memory unsafe so users aren't prepared to look out for these things. |
|
I experimented with fixing the thread sanitizer issues again, this time using NIOConcurrencyHelpers where possible (eg. dealing with simple Bool / Int values). I addressed each property individually as a sequence of commits here: https://github.com/IBM-Swift/Kitura-net/compare/concurrencyFixes2 These are cumulative, so compare each line with the previous. Error margin is quite high (+/- 1% or so). I did a subsequent experiment focusing on just the Socket file descriptor where I modified BlueSocket to use a NIO Atomic for Socket.socketfd, and this was an improvement, though still much slower than base: Current thoughts are around the handling of the socketfd value and whether we could stash a constant value for it when creating the |
|
I have an app that also appears to crash "at random" with |
|
@hactar I'd recommend using https://github.com/ianpartridge/swift-backtrace to get a better crash trace. Fwiw, my app has been running in production for a few months without crashing so I don't think these data races are causing issues very often. My app is mostly websockets which are long-lived rather than serving a lot of new connections, so your mileage may vary. |
I tried running my server with thread sanitizer enabled and it flagged several data races in InconingSocketManager.swift. My server uses websockets, which might be related to these data races.
They are easy to repro for me, but here are some of them it found for reference:
The text was updated successfully, but these errors were encountered: