Patchwork Fix libbacktrace on 32-bit sparc

login
register
mail settings
Submitter David Miller
Date Oct. 27, 2012, 4:27 a.m.
Message ID <20121027.002724.1098296946918586594.davem@davemloft.net>
Download mbox | patch
Permalink /patch/194601/
State New
Headers show

Comments

David Miller - Oct. 27, 2012, 4:27 a.m.
I'm getting a SIGBUS on every backtrace libbacktrace generates
on 32-bit sparc builds.  The crashes usually happen in
add_function_range(), where 'p' is not 8-byte aligned.

It seems that the vector code doesn't take care to align the pointers
it returns.  I cribbed the size alignment done in mmap.c's
implementation of backtrace_alloc() to fix this.

Ok to install?

libbacktrace/

2012-10-26  David S. Miller  <davem@davemloft.net>

	* alloc.c (backtrace_vector_grow): Round size up to a multiple
	of 8.
	* mmap.c (backtrace_vector_grow): Likewise.
Ian Taylor - Oct. 28, 2012, 4:06 a.m.
On Fri, Oct 26, 2012 at 9:27 PM, David Miller <davem@davemloft.net> wrote:
>
> I'm getting a SIGBUS on every backtrace libbacktrace generates
> on 32-bit sparc builds.  The crashes usually happen in
> add_function_range(), where 'p' is not 8-byte aligned.
>
> It seems that the vector code doesn't take care to align the pointers
> it returns.  I cribbed the size alignment done in mmap.c's
> implementation of backtrace_alloc() to fix this.

Sorry about the problem, but I don't see how this can be the right
fix.  A single vector will always be an array of the same struct, so I
don't see how any individual struct can be misaligned.  It seems like
increasing the requested size is just going to consistently misalign
any struct that does not require 8 byte alignment, so later references
into the vector using an index will fail.  Also backtrace_vector_grow
is based on top of backtrace_alloc, so again aligning the size
shouldn't matter.

The struct used  by add_function_range is

struct function_addrs
{
  uint64_t low;
  uint64_t high;
  struct function *function;
};

So on a 32-bit system, this should have a size of 20 if uint64_t
requires 4-byte alignment, but it should have a size of 24 if uint64_t
requires 8-byte alignment.  It sounds like uint64_t requires 8-byte
alignment, so the size of this struct should be 24, so your patch
shouldn't change matters.  Since your patch presumably works, it
sounds like sizeof (struct function_addrs) is returning 20, but that
does not make sense.  It would mean that allocating an array of struct
function_addrs wouldn't work correctly.

So I don't know what is going on.

Ian
David Miller - Oct. 28, 2012, 4:31 a.m.
From: Ian Lance Taylor <iant@google.com>
Date: Sat, 27 Oct 2012 21:06:59 -0700

> The struct used  by add_function_range is
> 
> struct function_addrs
> {
>   uint64_t low;
>   uint64_t high;
>   struct function *function;
> };
> 
> So on a 32-bit system, this should have a size of 20 if uint64_t
> requires 4-byte alignment, but it should have a size of 24 if uint64_t
> requires 8-byte alignment.  It sounds like uint64_t requires 8-byte
> alignment, so the size of this struct should be 24, so your patch
> shouldn't change matters.  Since your patch presumably works, it
> sounds like sizeof (struct function_addrs) is returning 20, but that
> does not make sense.  It would mean that allocating an array of struct
> function_addrs wouldn't work correctly.
> 
> So I don't know what is going on.

The size is 24, and my patch definitely makes the crashes go away.

It seems like a vector is being used for a mixed set of objects.
I'll try to figure out how that is happening.
David Miller - Oct. 28, 2012, 5:12 a.m.
From: David Miller <davem@davemloft.net>
Date: Sun, 28 Oct 2012 00:31:27 -0400 (EDT)

> The size is 24, and my patch definitely makes the crashes go away.
> 
> It seems like a vector is being used for a mixed set of objects.
> I'll try to figure out how that is happening.

Ok, the problem seems to have to do with releases.

The releases place vector memory chunks into a global pool.

So a memory chunk from a vector used for one type of object,
can be sucked into and used by another vector.

But the alignment requirements are different, so we can
obtain a chunk from the freelist that was being used for
a vector of 4-byte aligned objects.

The crash sequences are always of the form:

vec_release(0xffb37ac8) base+size(0xf0199008) amount(312)
...
vec_grow(0xffb37ac8:24) from 0x975168, ret=0xf01754cc [size(24):alc(360)]

That size alignment done by backtrace_alloc() has no influence upon
this issue.  Since chunks are released from wherever the vector's
allocation point was at the time of the release.

In fact I bet that alignment in backtrace_alloc() never triggers when
it is invoked from backtrace_vector_grow().
Ian Taylor - Oct. 29, 2012, 3:43 p.m.
On Sat, Oct 27, 2012 at 10:12 PM, David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Sun, 28 Oct 2012 00:31:27 -0400 (EDT)
>
>> The size is 24, and my patch definitely makes the crashes go away.
>>
>> It seems like a vector is being used for a mixed set of objects.
>> I'll try to figure out how that is happening.
>
> Ok, the problem seems to have to do with releases.
>
> The releases place vector memory chunks into a global pool.
>
> So a memory chunk from a vector used for one type of object,
> can be sucked into and used by another vector.
>
> But the alignment requirements are different, so we can
> obtain a chunk from the freelist that was being used for
> a vector of 4-byte aligned objects.
>
> The crash sequences are always of the form:
>
> vec_release(0xffb37ac8) base+size(0xf0199008) amount(312)
> ...
> vec_grow(0xffb37ac8:24) from 0x975168, ret=0xf01754cc [size(24):alc(360)]
>
> That size alignment done by backtrace_alloc() has no influence upon
> this issue.  Since chunks are released from wherever the vector's
> allocation point was at the time of the release.
>
> In fact I bet that alignment in backtrace_alloc() never triggers when
> it is invoked from backtrace_vector_grow().

Thanks for tracking it down.

This patch should fix it.  Bootstrapped and ran libbacktrace testsuite
on x86_64-unknown-linux-gnu.  Committed to mainline.

Ian


2012-10-29  Ian Lance Taylor  <iant@google.com>

	* mmap.c (backtrace_vector_release): Make sure freed block is
	aligned on 8-byte boundary.
David Miller - Oct. 29, 2012, 6:03 p.m.
From: Ian Lance Taylor <iant@google.com>
Date: Mon, 29 Oct 2012 08:43:42 -0700

> This patch should fix it.  Bootstrapped and ran libbacktrace testsuite
> on x86_64-unknown-linux-gnu.  Committed to mainline.

I can tell just by looking at your patch that it doesn't fix the
problem.

It's "vec->base + vec->size" that's not aligned, rather than the
length.  All your change does it align the length term.

You have to align the vec->size term in the pointer computation.

Actually you need to align both.

First you have to align vec->size to an 8 byte boundary.  Then
you have to align the remaining length that results to an 8
byte boundary as well.

Can you post a test patch for me this time, instead of just committing
a change which hasn't been tested to fix the problem?  I'd very much
appreciate that.

Thanks.
Ian Taylor - Oct. 29, 2012, 6:13 p.m.
On Mon, Oct 29, 2012 at 11:03 AM, David Miller <davem@davemloft.net> wrote:
> From: Ian Lance Taylor <iant@google.com>
> Date: Mon, 29 Oct 2012 08:43:42 -0700
>
>> This patch should fix it.  Bootstrapped and ran libbacktrace testsuite
>> on x86_64-unknown-linux-gnu.  Committed to mainline.
>
> I can tell just by looking at your patch that it doesn't fix the
> problem.
>
> It's "vec->base + vec->size" that's not aligned, rather than the
> length.  All your change does it align the length term.
>
> You have to align the vec->size term in the pointer computation.

I did.  I changed it from vec->base + vec->size.  Now it is vec->base
+ size, where size is the aligned version of vec->size.  I know that
vec->base is aligned.  Now that size is aligned, I know that vec->base
+ size is aligned.

> First you have to align vec->size to an 8 byte boundary.  Then
> you have to align the remaining length that results to an 8
> byte boundary as well.

Yes, the patch does both of those things.

> Can you post a test patch for me this time, instead of just committing
> a change which hasn't been tested to fix the problem?  I'd very much
> appreciate that.

Sure, if you can confirm that this patch fails, I will take another look.

Ian
David Miller - Oct. 29, 2012, 6:14 p.m.
From: Ian Lance Taylor <iant@google.com>
Date: Mon, 29 Oct 2012 11:13:19 -0700

> Sure, if you can confirm that this patch fails, I will take another look.

It fails.

Same problem, SIGBUS.
David Miller - Oct. 29, 2012, 6:15 p.m.
From: Ian Lance Taylor <iant@google.com>
Date: Mon, 29 Oct 2012 11:13:19 -0700

> I changed it from vec->base + vec->size.  Now it is vec->base
> + size, where size is the aligned version of vec->size.

It is not.

You don't modify 'size' at all.

Patch

diff --git a/libbacktrace/alloc.c b/libbacktrace/alloc.c
index 501f386..59072ed 100644
--- a/libbacktrace/alloc.c
+++ b/libbacktrace/alloc.c
@@ -78,6 +78,10 @@  backtrace_vector_grow (struct backtrace_state *state ATTRIBUTE_UNUSED,
 {
   void *ret;
 
+  /* Round for alignment; we assume that no type we care about
+     is more than 8 bytes.  */
+  size = (size + 7) & ~ (size_t) 7;
+
   if (size > vec->alc)
     {
       size_t alc;
diff --git a/libbacktrace/mmap.c b/libbacktrace/mmap.c
index e07810d..6e51a0d 100644
--- a/libbacktrace/mmap.c
+++ b/libbacktrace/mmap.c
@@ -175,6 +175,10 @@  backtrace_vector_grow (struct backtrace_state *state,size_t size,
 {
   void *ret;
 
+  /* Round for alignment; we assume that no type we care about
+     is more than 8 bytes.  */
+  size = (size + 7) & ~ (size_t) 7;
+
   if (size > vec->alc)
     {
       size_t pagesize;