[PATCH] ORBit memory alignment bug

[Hans Petter Jansson <hpj ximian com>]

ORBit's giop-recv-buffers rely on malloc() returning 8-byte aligned
memory. This is true for Linux/glibc/i386, but other allocators,
particularly valgrind's, align to 4 bytes.

The assertion that checks the alignment is wrong, which means that when
the marshaler and demarshaler get memory blocks aligned to 4 and 8 bytes
respectively, they will interpret the marshaled data differently (since
alignment is relative to the block's pointer, not the offset into the
block), leading to arbitrary behaviour and crashes.

I have a diff against the gnome-2-0 branch that makes giop-recv-buffers
handle 4-byte alignment gracefully. It's attached. If it's good enough,
can someone apply it to the relevant branches? I can also apply it
myself if someone approves it and specifies which branches to apply to.

[Since I'm adding a field to GIOPRecvBuffer, and this struct is public,
it will lead to binary incompatibility if applications use that struct
directly. Is this a concern?]

I'd be very happy if this makes it in, since we can then valgrind apps
that use CORBA (everything that relies on gconf, for instance). It works
perfectly when the patch is applied.

-- 
Hans Petter
Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/ORBit2/ChangeLog,v
retrieving revision 1.413.4.2
diff -u -r1.413.4.2 ChangeLog
--- ChangeLog	20 Sep 2002 04:22:45 -0000	1.413.4.2
+++ ChangeLog	3 Oct 2002 16:25:36 -0000
@@ -1,3 +1,12 @@
+2002-10-03  Hans Petter Jansson  <hpj@ximian.com>
+
+	* src/orb/GIOP/giop-recv-buffer.c (alloc_buffer): Handle 4-byte
+	aligned malloc() robustly, by aligning to 8 bytes when necessary.
+	(giop_recv_buffer_unuse): Free unaligned buffers correctly.
+
+	* include/orbit/GIOP/giop-recv-buffer.h: Add pointer to the real
+	allocated buffer memory to GIOPRecvBuffer.
+
 2002-09-20  Mark McLoughlin  <mark@skynet.ie>
 
 	* ORBit.spec.in: remove name-client-2 and
Index: include/orbit/GIOP/giop-recv-buffer.h
===================================================================
RCS file: /cvs/gnome/ORBit2/include/orbit/GIOP/giop-recv-buffer.h,v
retrieving revision 1.25
diff -u -r1.25 giop-recv-buffer.h
--- include/orbit/GIOP/giop-recv-buffer.h	1 Jul 2002 11:08:17 -0000	1.25
+++ include/orbit/GIOP/giop-recv-buffer.h	3 Oct 2002 16:25:36 -0000
@@ -42,6 +42,7 @@
 struct _GIOPRecvBuffer {
 	GIOPMsg msg;
 
+	guchar *message_body_mem;
 	guchar *message_body;
 	guchar *cur;
 	guchar *end;
Index: src/orb/GIOP/giop-recv-buffer.c
===================================================================
RCS file: /cvs/gnome/ORBit2/src/orb/GIOP/giop-recv-buffer.c,v
retrieving revision 1.56
diff -u -r1.56 giop-recv-buffer.c
--- src/orb/GIOP/giop-recv-buffer.c	5 Aug 2002 11:58:21 -0000	1.56
+++ src/orb/GIOP/giop-recv-buffer.c	3 Oct 2002 16:25:40 -0000
@@ -467,8 +467,8 @@
 		return;
 
 	if (buf->free_body) {
-		g_free (buf->message_body);
-		buf->message_body = NULL;
+		g_free (buf->message_body_mem);
+		buf->message_body = buf->message_body_mem = NULL;
 	}
 
 	switch (buf->giop_version) {
@@ -795,17 +795,44 @@
 static gboolean
 alloc_buffer (GIOPRecvBuffer *buf, gpointer old_alloc, gulong body_size)
 {
-	buf->message_body = g_try_realloc (old_alloc, body_size + 12);
+	gpointer old_body = buf->message_body;
+
+	/* We allocate 4 bytes extra, so we can use it to align to an 8-byte
+	 * boundary if necessary. */
+	buf->message_body = buf->message_body_mem =
+		g_try_realloc (old_alloc == old_body ?
+			       buf->message_body_mem : old_alloc,
+			       body_size + 12 + 4);
 
 	if (!buf->message_body)
 		return TRUE;
 
 	/*
-	 *   We assume that this is 8 byte aligned, for efficiency -
-	 * so we can align to the memory address rather than the offset
-	 * into the buffer.
+	 *   We assume that this is at least 4-byte aligned. We enforce
+	 * 8-byte alignment for efficiency - so we can align to the memory
+	 * address rather than the offset into the buffer.
 	 */
 	g_assert (((gulong)buf->message_body & 0x3) == 0);
+
+	if ((gulong) buf->message_body & 0x7) {
+		/* If new address is 4-byte aligned and old address was 8-byte
+		 * aligned, move data up to retain alignment. */
+		if (!((gulong) old_alloc & 0x7)) {
+			memmove (buf->message_body + 4, buf->message_body,
+				 body_size + 12);
+		}
+		buf->message_body = buf->message_body_mem + 4;
+	} else if (old_alloc == old_body) {
+		/* If new address is 8-byte aligned and old address was 4-byte
+		 * aligned, move data down to retain alignment. We can only do
+		 * this if we provided the old address (which means we moved
+		 * the data up previously). */
+		if ((gulong) old_alloc & 0x7) {
+			memmove (buf->message_body, buf->message_body + 4,
+				 body_size + 12);
+		}
+	}
+
 	buf->free_body = TRUE;
 	buf->cur = buf->message_body + 12;
 	buf->end = buf->cur + body_size;