[1/3] setjmp: fix x86-64 longjmp argument adjustment

Submitted by Alexander Monakov on Aug. 11, 2020, 6:11 p.m.

Details

Message ID 20200811181116.8433-1-amonakov@ispras.ru
State New
Series "Series without cover letter"
Headers show

Commit Message

Alexander Monakov Aug. 11, 2020, 6:11 p.m.
longjmp 'val' argument is an int, but the assembly is referencing 64-bit
registers as if the argument was a long, or the caller was responsible
for extending the argument. Though the psABI is not clear on this, the
interpretation in GCC is that high bits may be arbitrary and the callee
is responsible for sign/zero-extending the value as needed (likewise for
return values: callers must anticipate that high bits may be garbage).

Therefore testing %rax is a functional bug: setjmp would wrongly return
zero if longjmp was called with val==0, but high bits of %rsi happened
to be non-zero.

Rewrite the prologue to refer to 32-bit registers. In passing, change
'test' to use %rsi, as there's no advantage to using %rax and the new
form is cheaper on processors that do not perform move elimination.
---
 src/setjmp/x32/longjmp.s    | 6 +++---
 src/setjmp/x86_64/longjmp.s | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/setjmp/x32/longjmp.s b/src/setjmp/x32/longjmp.s
index e175a4b9..e709acad 100644
--- a/src/setjmp/x32/longjmp.s
+++ b/src/setjmp/x32/longjmp.s
@@ -5,10 +5,10 @@ 
 .type longjmp,@function
 _longjmp:
 longjmp:
-	mov %rsi,%rax           /* val will be longjmp return */
-	test %rax,%rax
+	mov %esi,%eax           /* val will be longjmp return */
+	test %esi,%esi
 	jnz 1f
-	inc %rax                /* if val==0, val=1 per longjmp semantics */
+	inc %eax                /* if val==0, val=1 per longjmp semantics */
 1:
 	mov (%rdi),%rbx         /* rdi is the jmp_buf, restore regs from it */
 	mov 8(%rdi),%rbp
diff --git a/src/setjmp/x86_64/longjmp.s b/src/setjmp/x86_64/longjmp.s
index e175a4b9..e709acad 100644
--- a/src/setjmp/x86_64/longjmp.s
+++ b/src/setjmp/x86_64/longjmp.s
@@ -5,10 +5,10 @@ 
 .type longjmp,@function
 _longjmp:
 longjmp:
-	mov %rsi,%rax           /* val will be longjmp return */
-	test %rax,%rax
+	mov %esi,%eax           /* val will be longjmp return */
+	test %esi,%esi
 	jnz 1f
-	inc %rax                /* if val==0, val=1 per longjmp semantics */
+	inc %eax                /* if val==0, val=1 per longjmp semantics */
 1:
 	mov (%rdi),%rbx         /* rdi is the jmp_buf, restore regs from it */
 	mov 8(%rdi),%rbp

Comments

Rich Felker Aug. 11, 2020, 6:45 p.m.
On Tue, Aug 11, 2020 at 09:11:14PM +0300, Alexander Monakov wrote:
> longjmp 'val' argument is an int, but the assembly is referencing 64-bit
> registers as if the argument was a long, or the caller was responsible
> for extending the argument. Though the psABI is not clear on this, the
> interpretation in GCC is that high bits may be arbitrary and the callee
> is responsible for sign/zero-extending the value as needed (likewise for
> return values: callers must anticipate that high bits may be garbage).
> 
> Therefore testing %rax is a functional bug: setjmp would wrongly return
> zero if longjmp was called with val==0, but high bits of %rsi happened
> to be non-zero.
> 
> Rewrite the prologue to refer to 32-bit registers. In passing, change
> 'test' to use %rsi, as there's no advantage to using %rax and the new
> form is cheaper on processors that do not perform move elimination.
> ---
>  src/setjmp/x32/longjmp.s    | 6 +++---
>  src/setjmp/x86_64/longjmp.s | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/src/setjmp/x32/longjmp.s b/src/setjmp/x32/longjmp.s
> index e175a4b9..e709acad 100644
> --- a/src/setjmp/x32/longjmp.s
> +++ b/src/setjmp/x32/longjmp.s
> @@ -5,10 +5,10 @@
>  .type longjmp,@function
>  _longjmp:
>  longjmp:
> -	mov %rsi,%rax           /* val will be longjmp return */
> -	test %rax,%rax
> +	mov %esi,%eax           /* val will be longjmp return */
> +	test %esi,%esi
>  	jnz 1f
> -	inc %rax                /* if val==0, val=1 per longjmp semantics */
> +	inc %eax                /* if val==0, val=1 per longjmp semantics */
>  1:
>  	mov (%rdi),%rbx         /* rdi is the jmp_buf, restore regs from it */
>  	mov 8(%rdi),%rbp
> diff --git a/src/setjmp/x86_64/longjmp.s b/src/setjmp/x86_64/longjmp.s
> index e175a4b9..e709acad 100644
> --- a/src/setjmp/x86_64/longjmp.s
> +++ b/src/setjmp/x86_64/longjmp.s
> @@ -5,10 +5,10 @@
>  .type longjmp,@function
>  _longjmp:
>  longjmp:
> -	mov %rsi,%rax           /* val will be longjmp return */
> -	test %rax,%rax
> +	mov %esi,%eax           /* val will be longjmp return */
> +	test %esi,%esi
>  	jnz 1f
> -	inc %rax                /* if val==0, val=1 per longjmp semantics */
> +	inc %eax                /* if val==0, val=1 per longjmp semantics */
>  1:
>  	mov (%rdi),%rbx         /* rdi is the jmp_buf, restore regs from it */
>  	mov 8(%rdi),%rbp
> -- 
> 2.11.0

Thanks! This whole series looks good and I'm applying it now.

Re: optimizing the tails, I wonder why they were ever written the way
they were to begin with. At least the i386 one was written by me so
there's nobody else to ask, but do you have any insight into what I
might have been thinking?

Rich
Alexander Monakov Aug. 12, 2020, 1:29 p.m.
On Tue, 11 Aug 2020, Rich Felker wrote:

> Thanks! This whole series looks good and I'm applying it now.
> 
> Re: optimizing the tails, I wonder why they were ever written the way
> they were to begin with. At least the i386 one was written by me so
> there's nobody else to ask, but do you have any insight into what I
> might have been thinking?

No idea.

By the way, you should check if other 64-bit ports in musl exhibit
the same issue. The AArch64 port definitely does, for example.

Here's a standalone testcase, needs -O2 so test_lj passes its second
argument unchanged:

#include <setjmp.h>

static void test_lj(jmp_buf jb, long lv)
{
        longjmp(jb, lv);
}

int main()
{
        void (*volatile ptest)(jmp_buf, long) = 0;
        jmp_buf jb;

        int v = setjmp(jb);
        ptest = ptest ? 0 : test_lj;
        if (ptest) ptest(jb, 1l<<32);
        return !v;
}


Alexander
Jens Gustedt Aug. 13, 2020, 8:01 a.m.
Alexander,
probably this has nothing to do with the issue at hand but 

on Wed, 12 Aug 2020 16:29:59 +0300 (MSK) you (Alexander Monakov
<amonakov@ispras.ru>) wrote:

>         int v = setjmp(jb);

is a use of `setjmp` that is not conforming. `setjmp` is only allowed
either standalone or as controlling expression either directly or
negated.

Jens
Alexander Monakov Aug. 13, 2020, 8:53 a.m.
On Thu, 13 Aug 2020, Jens Gustedt wrote:

> Alexander,
> probably this has nothing to do with the issue at hand but 
> 
> on Wed, 12 Aug 2020 16:29:59 +0300 (MSK) you (Alexander Monakov
> <amonakov@ispras.ru>) wrote:
> 
> >         int v = setjmp(jb);
> 
> is a use of `setjmp` that is not conforming. `setjmp` is only allowed
> either standalone or as controlling expression either directly or
> negated.

Thanks. Yeah, this is a moot point as the test needs GCC with -O anyway, and
GCC allows setjmp in any context. But nevertheless it's easy to adjust it
by changing the offending line to 'if (setjmp(jb)) return 0;' and changing
the final 'return !v'  to 'return 1'.

Alexander