Bug#856487: libsbc1: compiling with gcc > 4.9 causes stack corruption

[Nobuhiro Iwamatsu]

Hi, all.

Thanks for your comments.

>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.
>
>Unfortunately this is not correct since extended asm is not allowed in
>naked functions.
>
>Short-term I'd suggest to use the attached patch, that disables the
>ARMv6 asm implementation and uses the C implementation instead.
>
>> 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).
>
>This is either a bug in gcc or insufficient documentation in gcc.
>

I see.
I will fix using --enable-high-precision of configure option.

>Could you (or did you already) submit that to the gcc bugzilla?

OK, I will report this to GCC.
>
>> Paul
>>...
>
>Thanks
>Adrian


Best regards,
  Nobuhiro

-- 
Nobuhiro Iwamatsu
   iwamatsu at {nigauri.org / debian.org}
   GPG ID: 40AD1FA6

Bug#856487: libsbc1: compiling with gcc > 4.9 causes stack corruption

[Adrian Bunk]

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.

Unfortunately this is not correct since extended asm is not allowed in 
naked functions.

Short-term I'd suggest to use the attached patch, that disables the 
ARMv6 asm implementation and uses the C implementation instead.

> 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).

This is either a bug in gcc or insufficient documentation in gcc.

Could you (or did you already) submit that to the gcc bugzilla?

> Paul
>...

Thanks
Adrian

-- 

   "Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
   "Only a promise," Lao Er said.
   Pearl S. Buck - Dragon Seed

Description: Disable the ARMv6 asm implementation
 This gets miscompiled with recent gcc since gcc does not consider
 r3 clobbered by the basic asm in a naked function (see #856487).
 .
 The imost simple short-term fix is to disable the ARMv6 asm
 implementation on armhf and use the C implementation instead.
Author: Adrian Bunk 
Bug-Debian: https://bugs.debian.org/856487

--- sbc-1.3.orig/sbc/sbc_primitives_armv6.h
+++ sbc-1.3/sbc/sbc_primitives_armv6.h
@@ -35,7 +35,7 @@
 	defined(__ARM_ARCH_6M__) || defined(__ARM_ARCH_7__) || \
 	defined(__ARM_ARCH_7A__) || defined(__ARM_ARCH_7R__) || \
 	defined(__ARM_ARCH_7M__)
-#define SBC_HAVE_ARMV6 1
+//#define SBC_HAVE_ARMV6 1
 #endif
 
 #if !defined(SBC_HIGH_PRECISION) && (SCALE_OUT_BITS == 15) && \

Bug#856487: libsbc1: compiling with gcc > 4.9 causes stack corruption

[Uwe Kleine-König]

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.0 +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 
>> #include 
>>
>> 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:

06cc :
 6cc:   e3a03102mov r3, #-2147483648; 0x8000
 6d0:   e5813000str r3, [r1]
 6d4:   e12fff1ebx  lr

06d8 :
 6d8:   e92d41f0push{r4, r5, r6, r7, r8, lr}
 6dc:   e1a04001mov r4, r1
 6e0:   e1a01002mov r1, r2
 6e4:   e59f2054ldr r2, [pc, #84]   ; 740 
 6e8:   e59f0054ldr r0, [pc, #84]   ; 744 
 6ec:   e08f2002add r2, pc, r2
 6f0:   e7925000ldr r5, [r2, r0]
 6f4:   e1a03103lsl r3, r3, #2
 6f8:   e1a02005mov r2, r5
 6fc:   e2840030add r0, r4, #48 ; 0x30
 700:   e0817003add r7, r1, r3
 704:   ebf0bl  6cc 
 708:   e1a01007mov r1, r7
 70c:   e0876003add r6, r7, r3
 710:   e1a02005mov r2, r5
 714:   e2840020add r0, r4, #32
 718:   ebebbl  6cc 
 71c:   e1a01006mov r1, r6
 720:   e1a02005mov r2, r5
 724:   e2840010add r0, r4, #16
 728:   ebe7bl  6cc 
 72c:   e1a02005mov r2, r5
 730:   e0861003add r1, r6, r3
 734:   e1a4mov r0, r4
 738:   e8bd41f0pop {r4, r5, r6, r7, r8, lr}
 73c:   eae2b   6cc 
 740:   0001090c.word   0x0001090c
 744:   0034.word   0x0034

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



signature.asc
Description: OpenPGP digital signature

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.0 +0100
> +++ sbc-1.3/sbc/sbc_primitives_armv6.c2017-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 
> #include 
> 
> 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, #0x8000\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" (0x8000), "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 = 0x8000;
}

and not bother about inline assembly.)

Best regards
Uwe


signature.asc
Description: PGP signature

Bug#856487: libsbc1: compiling with gcc > 4.9 causes stack corruption

[Niels Thykier]

On Mon, 17 Apr 2017 17:02:32 +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.
> 

Hi Paul,

Many thanks for finding the bug and the solution to it.

> The attached patch works around the bug.
> 

@Bluetooth maintainers: Could you please apply the patch below for stretch?

> 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).
> 
> Paul
> 

@ARM porters: I will let you be the judge of this and forward it to GCC
as necessary.  (The test case is available from:
https://bugs.debian.org/cgi-bin/bugreport.cgi?att=1;bug=856487;filename=gcc-naked-bug.c;msg=130)

Thanks,
~Niels


> 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.0 +0100
> +++ sbc-1.3/sbc/sbc_primitives_armv6.c2017-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"
>   );
>  }
>  
> @@ -258,6 +259,7 @@
>   "pop{r8-r11}\n"
>   "stmia  r1!, {r4, r5, r6, r7}\n"
>   "pop{r1, r4-r7, pc}\n"
> +:::"r0", "r2", "r3", "ip"
>   );
>  }
>  

Bug#856487: libsbc1: compiling with gcc > 4.9 causes stack corruption

[Paul Brook]

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).

Paul

diff -ur clean/sbc/sbc_primitives_armv6.c sbc-1.3/sbc/sbc_primitives_armv6.c
--- clean/sbc/sbc_primitives_armv6.c2013-04-30 17:19:23.0 +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"
);
 }
 
@@ -258,6 +259,7 @@
"pop{r8-r11}\n"
"stmia  r1!, {r4, r5, r6, r7}\n"
"pop{r1, r4-r7, pc}\n"
+:::"r0", "r2", "r3", "ip"
);
 }
 
/* Compile with -O2 on arm */
#include 
#include 

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?
   gcc6 seems to look into the naked function and assume r3 is 
preserved accross the call.  */
__asm__ volatile ("mov r3, #0x8000\n\t"
"str r3, [r1]\n\t"
"bx lr"
#if 0
:::"r3"
#endif
);
}

int16_t c[4];
struct sbc_encoder_state;

void test(struct sbc_encoder_state *state,
int16_t *x, int32_t *out, int out_stride)
{
frob(x + 24, out, c);
out += out_stride;
frob(x + 16, out, c);
out += out_stride;
frob(x + 8, out, c);
out += out_stride;
frob(x + 0, out, c);
}

int main()
{
static int16_t a[32];
static int32_t b[32];
test(NULL, a, b, 8);
return 0;
}

Bug#856487: libsbc1: compiling with gcc > 4.9 causes stack corruption (on arm{el,hf}-only?)

[Niels Thykier]

On Sun, 19 Mar 2017 18:51:00 +0100 Linus =?utf-8?Q?L=C3=BCssing?=
 wrote:
> On Sat, Mar 04, 2017 at 07:37:36PM -0300, Felipe Sateler wrote:
> > > Not really familiar with how binaries get created or uploaded in
> > > Debian, but is it possible to determine the gcc + binutils
> > > versions with which libsbc 1.3-1 and 1.3-1+b2 were created? Just
> > > to double check whether the official uploads were indeed created
> > > with gcc-4.9 for libsbc 1.3-1 and gcc-5/gcc-6 for 1.3-1+b2?
> > 
> > The build logs are publicly available, for this build[1] the versions used 
> > were:
> > 
> > binutils_2.25-8
> > gcc-4.9_4.9.2-19
> > 
> > [1] 
> > https://buildd.debian.org/status/fetch.php?pkg=sbc=armhf=1.3-1=1433137735=0
> 
> Aiy, ok, thanks a lot, Felipe!
> 
> Is there anything else I could do? Was the issue reported to the
> gcc folks somewhere yet or should I report it to some bugtracker of
> the gcc project?
> 
> 

AFAICT, so we only have signs of arm being affected, but the arm port
list have not been included on this bug.  I am CC'ing them now so we are
certain that they are aware of it.

At the same time, has anyone reproduced this on any other architecture
than arm?

Thanks,
~Niels

Bug#856487: libsbc1: compiling with gcc > 4.9 causes stack corruption

[Dr. Johann Pfefferl]

Package: libsbc1
Version: 1.3-1+b2
Followup-For: Bug #856487

Dear Maintainer,

if I connect my bluetooth headphone to the machine and switch audio to
the bluetooth device pulseaudio crashes. The function shown by gdb is
"sbc_analyze_eight_armv6" from the libsbc library.

Now if I replace the original debian library by the following:

git clone git://git.debian.org/pkg-bluetooth/sbc.git
cd sbc
CC=gcc-4.9 ./configure
make
DESTDIR=$PWD/installdir
make install DESTDIR=$DESTDIR
install -v $DESTDIR/usr/local/lib/libsbc.so.1.2.1 
/usr/lib/arm-linux-gnueabihf/libsbc.so.1.2.1

and restart pulseaudio by

pactl exit;pactl info

and afterwards switch on my bluetooth headset and connect to the headset
by pavucontrol then everything works as expected.

If I do the same with CC=gcc-6 pulseaudio crashes.

The function sbc_analyze_eight_armv6 is an assembler function without
any comment :(. It seems that the calling convention of this direct
assembler routine has changed from gcc verion 4 to 6.

-- System Information:
Debian Release: 9.0
  APT prefers testing
  APT policy: (500, 'testing')
Architecture: armhf (armv7l)

Kernel: Linux 4.10.1-00038-g3d0309a
Locale: LANG=en_GB.UTF-8, LC_CTYPE=en_GB.UTF-8 (charmap=UTF-8)
Shell: /bin/sh linked to /bin/dash
Init: systemd (via /run/systemd/system)

Versions of packages libsbc1 depends on:
ii  libc6  2.24-9

libsbc1 recommends no packages.

libsbc1 suggests no packages.

-- no debconf information