futex: Call unset_alarm() before freeing the awaiter
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 12 Oct 2018 15:10:05 +0000 (11:10 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Fri, 12 Oct 2018 15:20:59 +0000 (11:20 -0400)
commit47c2cf2efd4ede802b233c285dd3ba09d338b4f2
treea811a24eadc6738b58b81581e1f3bf0a1bab8be7
parent73001cbb86fe9ff9272c45881595bd2fc482a8b9
futex: Call unset_alarm() before freeing the awaiter

The old 'awaiter->data = NULL' was a signal that we were done with the
awaiter.  The race was that the uthread that called futex_wait() was
restarted before everyone was done with the awaiter - where 'everyone'
means the timeout handler and futex_wake().  The last thing you do to
the awaiter was unblock it.

However, since commit 9969434fee6b ("parlib: Run alarm handlers outside
the tchain lock"), the alarm service is not done with the awaiter until
is_running == false and the condvar is unlocked.  The awaiter->data ==
NULL check was no longer sufficient.

Unsetting the alarm fixes this, just like the CV code does.  Though I
have broader concerns about this and other uses of set_alarm().  In
particular, it's a little lousy to unset alarms you know have completed,
since 'unset' is how you know the alarm is done.

The specific bug was a deadlock.  The alarm service ran the timeout
handler, but didn't grab the CV lock in the awaiter yet.  The uthread
that called futex_wake() unblocked, was run, returned, and then called
other functions (from Go / KenC btw).  That clobbered the awaiter, which
was on the stack.  The VC that ran the alarm handler then tried to grab
the CV lock, which happened to be clobbered with a function return
address.  The actual clobber was to set the lock 0, which meant that VC
0 was the lockholder.  VC 1 would just spin, checking that VC 0 was not
preempted.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
user/pthread/futex.c