Re: fix alignment issue on SPARC processors

Uli Schlachter <[email protected]>
Newsgroups gmane.comp.lib.cairo
Message-ID <[email protected]>
Hi Nicolas,

Am 04.12.2014 um 20:44 schrieb Nicolas Setton:
[...]
>> I would instead suggest to add a new member "int64_t data" to the struct with a
>> suitable comment explaining why it is of type int64_t. Instead of using
>> ((unsigned char *)chunk + sizeof(*chunk)) for calculating the beginning of the
>> data area of the chunk, the code could then use (unsigned char*)&chunk->data
>> which IMO is more readable anyway.
> 
> I didn't understand this suggestion.

Could you test the attached patch? Also, this is pretty much the idea that I had
in mind. Compared to your patch, it does not add a hole to struct _pool_chunk on
64bit platforms (in fact, nothing should change at all on 64bit).

I basically did no testing at all on this. The test suite run on the image
backend does not crash. So it would also be nice if others could take a look at
this and spot my mistakes. Pointer arithmetic is scary.

Cheers,
Uli
-- 
"In the beginning the Universe was created. This has made a lot of
 people very angry and has been widely regarded as a bad move."

-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo
0001-tor-scan-converter-Correctly-align-64bit-types.patch (text/x-diff, 5.6 KB)
From 9f318fc51963c221faf26b2877804d106a31b3e9 Mon Sep 17 00:00:00 2001
From: Uli Schlachter <[email protected]>
Date: Fri, 5 Dec 2014 14:43:26 +0100
Subject: [PATCH] tor-scan-converter: Correctly align 64bit types

On 32bit SPARC the scan converter was causing a SIGBUS due to an unaligned
memory access while accessing an int64_t. This memory accessing was to struct
quorem's rem member.

This crash occurred because the rot-scan-converter contains its own
implementation of a memory pool. This implementation only guarantees an
alignment of sizeof(void *), which is less than what a 64 bit type requires on
32bit platforms. This 4 byte alignment is guaranteed, because struct _pool_chunk
(which is the struct that is used for managing free space) contains elements of
that size and so the size of that struct is a multiple of this size as well.

This problem was introduced with commit 03c3d4b7c15.

To fix this problem, this commit introduces a int64_t member to struct
_pool_chunk that marks the beginning of the free data space. Thanks to this, the
compiler ensures proper alignment and sizeof(struct _pool_chunk) becomes a
multiple of 8.

However, previously the end of the struct marked the beginning of the data and
sizeof() was used for correctly calculating offsets to the data section. So,
just adding such a member would work, but would also waste some memory. To avoid
this, this commit also changes the rest of the pool implementation to
accommodate.

Reported-by: Nicolas Setton <[email protected]>
Signed-off-by: Uli Schlachter <[email protected]>
---
 src/cairo-tor-scan-converter.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/src/cairo-tor-scan-converter.c b/src/cairo-tor-scan-converter.c
index 14922d0..7adb5ab 100644
--- a/src/cairo-tor-scan-converter.c
+++ b/src/cairo-tor-scan-converter.c
@@ -277,7 +277,8 @@ struct _pool_chunk {
      * chunk in the pool header. */
     struct _pool_chunk *prev_chunk;
 
-    /* Actual data starts here.	 Well aligned for pointers. */
+    /* Actual data starts here. Well aligned even for 64 bit types. */
+    int64_t data;
 };
 
 /* A memory pool.  This is supposed to be embedded on the stack or
@@ -299,8 +300,11 @@ struct pool {
 
     /* Header for the sentinel chunk.  Directly following the pool
      * struct should be some space for embedded elements from which
-     * the sentinel chunk allocates from. */
-    struct _pool_chunk sentinel[1];
+     * the sentinel chunk allocates from. This is expressed as a char
+     * array so that the 'int64_t data' member of _pool_chunk isn't
+     * included. This way embedding struct pool in other structs works
+     * without wasting space. */
+    char sentinel[sizeof(struct _pool_chunk) - sizeof(int64_t)];
 };
 
 /* A polygon edge. */
@@ -474,8 +478,9 @@ static struct _pool_chunk *
 _pool_chunk_create(struct pool *pool, size_t size)
 {
     struct _pool_chunk *p;
+    size_t sizeof_chunk = sizeof(struct _pool_chunk) - sizeof(int64_t);
 
-    p = malloc(size + sizeof(struct _pool_chunk));
+    p = malloc(sizeof_chunk + size);
     if (unlikely (NULL == p))
 	longjmp (*pool->jmp, _cairo_error (CAIRO_STATUS_NO_MEMORY));
 
@@ -489,10 +494,10 @@ pool_init(struct pool *pool,
 	  size_t embedded_capacity)
 {
     pool->jmp = jmp;
-    pool->current = pool->sentinel;
+    pool->current = (void*) &pool->sentinel[0];
     pool->first_free = NULL;
     pool->default_capacity = default_capacity;
-    _pool_chunk_init(pool->sentinel, NULL, embedded_capacity);
+    _pool_chunk_init(pool->current, NULL, embedded_capacity);
 }
 
 static void
@@ -502,7 +507,7 @@ pool_fini(struct pool *pool)
     do {
 	while (NULL != p) {
 	    struct _pool_chunk *prev = p->prev_chunk;
-	    if (p != pool->sentinel)
+	    if (p != (void *) &pool->sentinel[0])
 		free(p);
 	    p = prev;
 	}
@@ -542,14 +547,14 @@ _pool_alloc_from_new_chunk(
 	chunk = _pool_chunk_create (pool, capacity);
     pool->current = chunk;
 
-    obj = ((unsigned char*)chunk + sizeof(*chunk) + chunk->size);
+    obj = ((unsigned char*)&chunk->data + chunk->size);
     chunk->size += size;
     return obj;
 }
 
 /* Allocate size bytes from the pool.  The first allocated address
- * returned from a pool is aligned to sizeof(void*).  Subsequent
- * addresses will maintain alignment as long as multiples of void* are
+ * returned from a pool is aligned to 8 bytes.  Subsequent
+ * addresses will maintain alignment as long as multiples of 8 are
  * allocated.  Returns the address of a new memory area or %NULL on
  * allocation failures.	 The pool retains ownership of the returned
  * memory. */
@@ -559,7 +564,7 @@ pool_alloc (struct pool *pool, size_t size)
     struct _pool_chunk *chunk = pool->current;
 
     if (size <= chunk->capacity - chunk->size) {
-	void *obj = ((unsigned char*)chunk + sizeof(*chunk) + chunk->size);
+	void *obj = ((unsigned char*)&chunk->data + chunk->size);
 	chunk->size += size;
 	return obj;
     } else {
@@ -573,16 +578,16 @@ pool_reset (struct pool *pool)
 {
     /* Transfer all used chunks to the chunk free list. */
     struct _pool_chunk *chunk = pool->current;
-    if (chunk != pool->sentinel) {
-	while (chunk->prev_chunk != pool->sentinel) {
+    if (chunk != (void *) &pool->sentinel[0]) {
+	while (chunk->prev_chunk != (void *) &pool->sentinel[0]) {
 	    chunk = chunk->prev_chunk;
 	}
 	chunk->prev_chunk = pool->first_free;
 	pool->first_free = pool->current;
     }
     /* Reset the sentinel as the current chunk. */
-    pool->current = pool->sentinel;
-    pool->sentinel->size = 0;
+    pool->current = (void *) &pool->sentinel[0];
+    pool->current->size = 0;
 }
 
 /* Rewinds the cell list's cursor to the beginning.  After rewinding
-- 
2.1.3
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.