Patchwork [16/17,v5] x86, pthread_cond_*wait: Do not depend on %eax not being clobbered

login
register
mail settings
Submitter Nix
Date March 13, 2016, 3:17 p.m.
Message ID <1457882222-22599-17-git-send-email-nix@esperi.org.uk>
Download mbox | patch
Permalink /patch/11328/
State New
Headers show

Comments

Nix - March 13, 2016, 3:17 p.m.
From: Nick Alcock <nick.alcock@oracle.com>

The x86-specific versions of both pthread_cond_wait and
pthread_cond_timedwait have (in their fall-back-to-futex-wait slow
paths) calls to __pthread_mutex_cond_lock_adjust followed by
__pthread_mutex_unlock_usercnt, which load the parameters before the
first call but then assume that the first parameter, in %eax, will
survive unaffected.  This happens to have been true before now, but %eax
is a call-clobbered register, and this assumption is not safe: it could
change at any time, at GCC's whim, and indeed the stack-protector canary
checking code clobbers %eax while checking that the canary is
uncorrupted.

So reload %eax before calling __pthread_mutex_unlock_usercnt.  (Do this
unconditionally, even when stack-protection is not in use, because it's
the right thing to do, it's a slow path, and anything else is dicing
with death.)

v5: New.

	* sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S: Reload
	call-clobbered %eax on retry path.
	* sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S: Likewise.
---
 sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S | 1 +
 sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S      | 1 +
 2 files changed, 2 insertions(+)
Florian Weimer - March 14, 2016, 8:01 p.m.
On 03/13/2016 04:17 PM, Nix wrote:
> From: Nick Alcock <nick.alcock@oracle.com>

> 	* sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S: Reload
> 	call-clobbered %eax on retry path.
> 	* sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S: Likewise.

Looks good.  Can you commit this yourself, or do you need someone to
commit it for you?

Thanks,
Florian
Nix - March 14, 2016, 11:36 p.m.
On 14 Mar 2016, Florian Weimer said:

> On 03/13/2016 04:17 PM, Nix wrote:
>> From: Nick Alcock <nick.alcock@oracle.com>
>
>> 	* sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S: Reload
>> 	call-clobbered %eax on retry path.
>> 	* sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S: Likewise.
>
> Looks good.  Can you commit this yourself, or do you need someone to
> commit it for you?

I don't have commit rights, so if you or someone else wants to commit
it, commit away!

(and if anyone has any more comments on the rest of the patch, do fire
away with slings and arrows etc. I'd hope it's converging on something
acceptable after this many rounds... :) )
Florian Weimer - March 23, 2016, 3:02 p.m.
On 03/15/2016 12:36 AM, Nix wrote:
> On 14 Mar 2016, Florian Weimer said:
> 
>> On 03/13/2016 04:17 PM, Nix wrote:
>>> From: Nick Alcock <nick.alcock@oracle.com>
>>
>>> 	* sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S: Reload
>>> 	call-clobbered %eax on retry path.
>>> 	* sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S: Likewise.
>>
>> Looks good.  Can you commit this yourself, or do you need someone to
>> commit it for you?
> 
> I don't have commit rights, so if you or someone else wants to commit
> it, commit away!

I have pushed this.

Thanks,
Florian
Nix - April 4, 2016, 10:09 p.m.
On 23 Mar 2016, Florian Weimer told this:

> On 03/15/2016 12:36 AM, Nix wrote:
>> On 14 Mar 2016, Florian Weimer said:
>> 
>>> On 03/13/2016 04:17 PM, Nix wrote:
>>>> From: Nick Alcock <nick.alcock@oracle.com>
>>>
>>>> 	* sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S: Reload
>>>> 	call-clobbered %eax on retry path.
>>>> 	* sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S: Likewise.
>>>
>>> Looks good.  Can you commit this yourself, or do you need someone to
>>> commit it for you?
>> 
>> I don't have commit rights, so if you or someone else wants to commit
>> it, commit away!
>
> I have pushed this.

Thank you!

... now, is there anything I can do to the rest to make people happier?
I think I've addressed all the review comments...

Patch

diff --git a/sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S b/sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S
index 96f8a8d..6256376 100644
--- a/sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S
+++ b/sysdeps/unix/sysv/linux/i386/pthread_cond_timedwait.S
@@ -297,6 +297,7 @@  __pthread_cond_timedwait:
 	   correctly.  */
 	movl	dep_mutex(%ebx), %eax
 	call	__pthread_mutex_cond_lock_adjust
+	movl	dep_mutex(%ebx), %eax
 	xorl	%edx, %edx
 	call	__pthread_mutex_unlock_usercnt
 	jmp	8b
diff --git a/sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S b/sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S
index 94302b0..5016718 100644
--- a/sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S
+++ b/sysdeps/unix/sysv/linux/i386/pthread_cond_wait.S
@@ -309,6 +309,7 @@  __pthread_cond_wait:
 	   correctly.  */
 	movl	dep_mutex(%ebx), %eax
 	call    __pthread_mutex_cond_lock_adjust
+	movl	dep_mutex(%ebx), %eax
 	xorl	%edx, %edx
 	call	__pthread_mutex_unlock_usercnt
 	jmp	8b