_backgroundPingTask use after free/ invalid thread check? #2186

Closed
dmoagx opened this Issue Jul 25, 2015 · 2 comments

Projects

None yet

2 participants

@dmoagx
Collaborator
dmoagx commented Jul 25, 2015

Looking at some crash reports there seems to be some threading issue here:

Thread 10:: SPMySQL connection keepalive thread
...
3   com.sequelpro.spmysql           0x0000000100370be0 -[SPMySQLConnection(Locking) _unlockConnection] + 44
4   com.sequelpro.spmysql           0x000000010036e638 -[SPMySQLConnection(Ping_and_KeepAlive) _pingConnectionUsingLoopDelay:] + 641
5   com.sequelpro.spmysql           0x000000010036e393 -[SPMySQLConnection(Ping_and_KeepAlive) _threadedKeepAlive] + 240
6   com.apple.Foundation            0x00007fff8c2addc2 __NSThread__main__ + 1345
7   libsystem_pthread.dylib         0x00007fff88662268 _pthread_body + 131
8   libsystem_pthread.dylib         0x00007fff886621e5 _pthread_start + 176
9   libsystem_pthread.dylib         0x00007fff8866041d thread_start + 13

Thread 11 Crashed:
0   com.sequelpro.spmysql           0x000000010036e6c5 _backgroundPingTask + 89
1   libsystem_pthread.dylib         0x00007fff88662268 _pthread_body + 131
2   libsystem_pthread.dylib         0x00007fff886621e5 _pthread_start + 176
3   libsystem_pthread.dylib         0x00007fff8866041d thread_start + 13

In code, the struct SPMySQLConnectionPingDetails *pingDetails has already been free()d by the time _unlockConnection is called. However _backgroundPingTask uses that struct, so it should no longer be running at that point.

I see three potential issues:

  • keepAlivePingThreadActive is changed from another thread but is not atomic (or using a barrier)
  • After calling pthread_kill keepAlivePingThreadActive is forced to NO but there is IMHO no guarantee the thread actually has exited
  • There is no mutex on thread setup/teardown which means pthread_kill/pthread_cancel could be called right when the thread itself is already exiting.

Reports:
http://log.sequelpro.com/view/4470
http://log.sequelpro.com/view/3811
http://log.sequelpro.com/view/4513

@dmoagx dmoagx added the Bug label Jul 25, 2015
@dmoagx
Collaborator
dmoagx commented Feb 21, 2016

Related commits:
4d97cbd
2b2a177

@dmoagx
Collaborator
dmoagx commented Feb 22, 2016

I'll assume this has been fixed by the previous commit, since we have no crash reports from nightly builds after that commit (whereas we had at least one per week in nightlies before that).

@dmoagx dmoagx added this to the 1.1.2 milestone Mar 6, 2016
@abhibeckert abhibeckert closed this Jun 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment