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

[Uwe Kleine-König]

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.

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. Also I'd
recommend to fix the prototype of the functions to match reality.

Other than that the code is hard to review and I wonder if it's worth to
have this as hand coded assembly. Also it maybe needs a "memory"
clobber?

But note that gcc inline assembly is a bit mysterious to me, so take my
comments with a grain of salt.

> /* 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.

>            gcc6 seems to look into the naked function and assume r3 is preserved accross the call.  */

No gcc rightly assumes that if r3 is clobbered, you'd say so.

> 	__asm__ volatile ("mov r3, #0x80000000\n\t"
> 			"str r3, [r1]\n\t"
> 			"bx lr"
> #if 0
> 			:::"r3"
> #endif
> 			);
> }

I'd code this as:

	static void frob(int16_t *a, int32_t *b, const int16_t *c)
	{
		__asm__ __volatile__ ("str %0, [%1]\n"
			: /* no output */
			: "r" (0x80000000), "r" (b)
			: "memory");
	}

I think the memory clobber is necessary as *b is modified.

(but actually no, I'd code this as:

	static void frob(int16_t *a, int32_t *b, const int16_t *c)
	{
		*b = 0x80000000;
	}

and not bother about inline assembly.)

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