EXC_BAD_ACCESS (SIGSEGV) crash reason
peternlewis opened this issue · 6 comments
// I was receiving about 3 EXC_BAD_ACCESS (SIGSEGV) crash reports a month that listed the 'path' objc_msgSend
A sequence which will generate this is:
VDKQueue deallocs
_keepWatcherThreadRunning = NO
[self removeAllPaths];
At that point all entries's are deallocated.
Meanwhile the thread is stuck in kevent
An event happens on the file
the thread reads the event, retrieves the dangling entry pointer from ev.udata
crash ensues
Multithreading is hard.
A bandaid solution would be to add
if ( !_keepWatcherThreadRunning ) break;
after the kevent, that will minimise the period where the dealloc can happen, but it will still be possible, just far less likely.
Really the events need to be retained by the thread as well and only released when the thread completes, or otherwise the entries need to be validated or looked for some other way that avoids leaving a dangling pointer.
Actually NSThread retains its target, so _keepWatcherThreadRunning will never be set to NO in dealloc because dealloc will never be called while the thread is running.
However the same race condition will occur if you remove a path any other way since the removal is not synced with the thread.
Holy hell, you’re totally right. Can’t believe I missed that. I can fix this in the next couple weeks, but if you’re willing to put together a PR I’d be happy to merge. Thanks!
On 24 Apr 2015, at 01:00, Peter N Lewis notifications@github.com wrote:
Actually NSThread retains its target, so _keepWatcherThreadRunning will never be set to NO in dealloc because dealloc will never be called while the thread is running.
However the same race condition will occur if you remove a path any other way since the removal is not synced with the thread.
—
Reply to this email directly or view it on GitHub #13 (comment).
I'm afraid I have trouble with github and pullrequests. I am entirely vandalizing the code (there is not much left really) for my purposes, but my solution for this issue went something like this:
static ptrint_t gUniqueIndex = 1;
each entry gets a ptrint_t_uniqueIndex, initialized to gUniqueIndex++
Add another NSMutableDictionary mapping uniqueIndex to entries.
Put the uniqueIndex in the event data field.
In the thread, within the @syncronized, look up the event based on the uniqueIndex.
My code is 64-bit only, so unique index will never wrap so that is never an issue.
And the thread already references self in other places, so avoiding referencing self is pointless (just make sure to use the @synronized).
I'm sure there are many other solutions, but I generally dislike keeping unsafe unretained references to objects and prefer for this sort of thing to use a safe index that looks to see if the object is still alive.
I'm happy to email the (as I say, highly vandalised) code to you to look at if you like (peter at stairways.com.au), though I haven't actually finished and got it working yet so no real guarantees, but it can show my thinking.
That’d be great. If you can send me a copy once you’re done with it, I’m @bdkjones on Twitter or bryan at incident 57 dot com.
On 24 Apr 2015, at 04:19, Peter N Lewis notifications@github.com wrote:
I'm afraid I have trouble with github and pullrequests. I am entirely vandalizing the code (there is not much left really) for my purposes, but my solution for this issue went something like this:
static ptrint_t gUniqueIndex = 1;
each entry gets a ptrint_t_uniqueIndex, initialized to gUniqueIndex++
Add another NSMutableDictionary mapping uniqueIndex to entries.
Put the uniqueIndex in the event data field.
In the thread, within the @syncronized https://github.com/syncronized, look up the event based on the uniqueIndex.
My code is 64-bit only, so unique index will never wrap so that is never an issue.
And the thread already references self in other places, so avoiding referencing self is pointless (just make sure to use the @synronized).
I'm sure there are many other solutions, but I generally dislike keeping unsafe unretained references to objects and prefer for this sort of thing to use a safe index that looks to see if the object is still alive.
I'm happy to email the (as I say, highly vandalised) code to you to look at if you like (peter at stairways.com.au), though I haven't actually finished and got it working yet so no real guarantees, but it can show my thinking.
—
Reply to this email directly or view it on GitHub #13 (comment).
@peternlewis if you're still monitoring this, is the patch I just posted along the lines of what you were saying? I'm using it in a current project and it seems to be working ok...
Yes, I had a quick look and it looks similar to what I did.