[Pkg-bluetooth-maintainers] Bug#856487: libsbc1: compiling with gcc > 4.9 causes stack corruption

Wed Apr 26 10:09:23 UTC 2017

Hello,

after some discussion in #debian-arm, I have to revise this.

On 04/26/2017 10:30 AM, Uwe Kleine-König wrote:
> On Mon, Apr 17, 2017 at 05:02:32PM +0100, Paul Brook wrote:
>> Package: libsbc1
>> Version: 1.3-1+b2
>> Followup-For: Bug #856487
>>
>> Not a stack corruption.
>>
>> This is miscompilation of sbc_analyze_4b_8s_armv6.  gcc appears to look
>> into the asm function and decides that it does not clobber r3 (which the
>> normal ARM ABI says is call clobbered).  The last out += out_stride ends
>> up incrementing the pointer by an arbitrary amount.
>>
>> The attached patch works around the bug.
>>
>> I'm not entirely sure whether this is a gcc bug or not, but at best it's
>> surprising behavior from gcc.  I've attached a reduced testcase for the toolchain
>> folks to argue over (compile with gcc -O2, tested with gcc 6.3.0-2 from
>> sid).
> 
> AFAICT this is not a gcc bug. gcc is not supposed (and I'm sure this is
> even impossible in general) to determine the set of clobbered registers.
> 
>> diff -ur clean/sbc/sbc_primitives_armv6.c sbc-1.3/sbc/sbc_primitives_armv6.c
>> --- clean/sbc/sbc_primitives_armv6.c	2013-04-30 17:19:23.000000000 +0100
>> +++ sbc-1.3/sbc/sbc_primitives_armv6.c	2017-04-17 16:43:49.918809345 +0100
>> @@ -102,6 +102,7 @@
>>  		"pop    {r8-r11}\n"
>>  		"stmia  r1, {r4, r5, r6, r7}\n"
>>  		"pop    {r1, r4-r7, pc}\n"
>> +        :::"r0", "r2", "r3", "ip"
> 
> While this might fix the problem at hand, this is not a complete fix.

naked functions must only use basic assembler according to

	https://gcc.gnu.org/onlinedocs/gcc/ARM-Function-Attributes.html

.

> So there is no guarantee that r0 still has the value passed to the
> function when it returns.
> 
> So r0, r1 and r2 must be declared inputs, r0 also an output.

So this is wrong.

> Also I'd
> recommend to fix the prototype of the functions to match reality.

This however stays correct.

> Other than that the code is hard to review and I wonder if it's worth to
> have this as hand coded assembly.

ditto.

>> /* Compile with -O2 on arm */
>> #include <stdint.h>
>> #include <stdlib.h>
>>
>> static void __attribute__((naked)) frob(int16_t *a, int32_t *b, const int16_t *c)
>> {
>> 	/* The explicit clobber of r3 should not be necessary because that it is implied by the function call?
> 
> No it's not.

ok, AAPCS[1] says:

	A subroutine must preserve the contents of the registers r4-
	r8, r10, r11 and SP (and r9 in PCS variants that
	designate r9 as v6).

So r3 can be clobbered and gcc must assume that this is the case.

BTW, the relevant assembly output (generated on abel.d.o) is:

000006cc <frob>:
 6cc:	e3a03102 	mov	r3, #-2147483648	; 0x80000000
 6d0:	e5813000 	str	r3, [r1]
 6d4:	e12fff1e 	bx	lr

000006d8 <test>:
 6d8:	e92d41f0 	push	{r4, r5, r6, r7, r8, lr}
 6dc:	e1a04001 	mov	r4, r1
 6e0:	e1a01002 	mov	r1, r2
 6e4:	e59f2054 	ldr	r2, [pc, #84]	; 740 <test+0x68>
 6e8:	e59f0054 	ldr	r0, [pc, #84]	; 744 <test+0x6c>
 6ec:	e08f2002 	add	r2, pc, r2
 6f0:	e7925000 	ldr	r5, [r2, r0]
 6f4:	e1a03103 	lsl	r3, r3, #2
 6f8:	e1a02005 	mov	r2, r5
 6fc:	e2840030 	add	r0, r4, #48	; 0x30
 700:	e0817003 	add	r7, r1, r3
 704:	ebfffff0 	bl	6cc <frob>
 708:	e1a01007 	mov	r1, r7
 70c:	e0876003 	add	r6, r7, r3
 710:	e1a02005 	mov	r2, r5
 714:	e2840020 	add	r0, r4, #32
 718:	ebffffeb 	bl	6cc <frob>
 71c:	e1a01006 	mov	r1, r6
 720:	e1a02005 	mov	r2, r5
 724:	e2840010 	add	r0, r4, #16
 728:	ebffffe7 	bl	6cc <frob>
 72c:	e1a02005 	mov	r2, r5
 730:	e0861003 	add	r1, r6, r3
 734:	e1a00004 	mov	r0, r4
 738:	e8bd41f0 	pop	{r4, r5, r6, r7, r8, lr}
 73c:	eaffffe2 	b	6cc <frob>
 740:	0001090c 	.word	0x0001090c
 744:	00000034 	.word	0x00000034

And at address 70c r3 is assumed to still hold the initial value but
frob already clobbered it when being called at address 704.

When I change frob to:

	void frob(int16_t *a, int32_t *b, const int16_t *c);

(and compile with -c) the code is correct. So it would indeed be
interesting to get some input from the gcc people.

Best regards
Uwe

[1]
http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.alioth.debian.org/pipermail/pkg-bluetooth-maintainers/attachments/20170426/fb707d59/attachment.sig>