Merge lp:~vanvugt/compiz-core/fix-940066 into lp:compiz-core

[Daniel van Vugt]

Avoid uninitialized memory reads, which could lead to unpredictable results.
The uninitialized memory was in the padding bytes of imperfectly aligned
structures. (LP: #940066)

[Sam Spilsbury (smspillaz)]

Gah, truly bitten by this I see :/

review: Approve

[Alan Griffiths (alan-griffiths)]

It seems odd to use "return memcmp (a, b, sizeof (double) * 6);" without at least a compile-time assert that sizeof (decor_matrix_t) == sizeof (double) * 6).

#define STATIC_ASSERT(condition, label) _Static_assert(condition, #label)
STATIC_ASSERT(sizeof (decor_matrix_t) == sizeof (double) * 6, decor_matrix_t is 6 doubles);

[Daniel van Vugt (vanvugt)]

That assertion would be incorrect, which is why I avoided sizeof (decor_matrix_t).

Structure alignment rules guarantee that decor_matrix_t contains 6 doubles tightly packed at offset 0. However I do not think they guarantee how much padding there is after that. Thus, you cannot assume sizeof (decor_matrix_t) == 6 * sizeof (double).

[Alan Griffiths (alan-griffiths)]

While I agree there is no C standard requirement that there is no tail
padding, the universal implementation of tail padding is for the size
of a struct to reach the smallest possible multiple of the alignment
required by the contained types.  (This is required by every ABI that
I've seen.)
If you feel a need to be paranoid I don't believe one can show that
all bits in a double are significant.

----- Original Message -----
From: <email address hidden>
To:"Daniel van Vugt"
Cc:
Sent:Sat, 25 Feb 2012 03:19:16 -0000
Subject:Re: [Merge] lp:~vanvugt/compiz-core/fix-940066 into
lp:compiz-core

 That assertion would be incorrect, which is why I avoided sizeof
(decor_matrix_t).

 Structure alignment rules guarantee that decor_matrix_t contains 6
doubles tightly packed at offset 0. However I do not think they
guarantee how much padding there is after that. Thus, you cannot
assume sizeof (decor_matrix_t) == 6 * sizeof (double).
 --
https://code.launchpad.net/~vanvugt/compiz-core/fix-940066/+merge/94502
[1]
 Your team Compiz Maintainers is subscribed to branch lp:compiz-core.

Links:
------
[1]
https://code.launchpad.net/~vanvugt/compiz-core/fix-940066/+merge/94502

[Daniel van Vugt (vanvugt)]

I think all the bits in a double are significant to an equality operation:
http://en.wikipedia.org/wiki/Double_precision_floating-point_format

But I'm happy to remove the memcmp for argument's sake.

[Sam Spilsbury (smspillaz)]

review: Approve

[Alan Griffiths (alan-griffiths)]

review: Approve

=== modified file 'libdecoration/decoration.c'
--- libdecoration/decoration.c	2012-02-09 07:48:57 +0000
+++ libdecoration/decoration.c	2012-02-27 06:48:18 +0000
@@ -369,6 +369,49 @@
     return n;
 }
 
+static int
+decor_point_cmp (const decor_point_t *a, const decor_point_t *b)
+{
+    /* Use binary | to avoid branch prediction slow-downs */
+    return (a->x - b->x) | (a->y - b->y) | (a->gravity - b->gravity);
+}
+
+static int
+decor_matrix_cmp (const decor_matrix_t *a, const decor_matrix_t *b)
+{
+    return (a->xx != b->xx) ||
+           (a->yx != b->yx) ||
+           (a->xy != b->xy) ||
+           (a->yy != b->yy) ||
+           (a->x0 != b->x0) ||
+           (a->y0 != b->y0);
+}
+
+static int
+decor_quad_cmp (const decor_quad_t *a, const decor_quad_t *b)
+{
+    return decor_point_cmp (&a->p1, &b->p1) ||
+           decor_point_cmp (&a->p2, &b->p2) ||
+           decor_matrix_cmp (&a->m, &b->m)  ||
+           (
+               (a->max_width  - b->max_width)  |
+               (a->max_height - b->max_height) |
+               (a->align      - b->align)      |
+               (a->clamp      - b->clamp)      |
+               (a->stretch    - b->stretch)
+           );
+}
+
+static int
+decor_extents_cmp (const decor_extents_t *a, const decor_extents_t *b)
+{
+    /* Use binary | to avoid branch prediction slow-downs */
+    return (a->left   - b->left)  |
+           (a->right  - b->right) |
+           (a->top    - b->top)   |
+           (a->bottom - b->bottom);
+}
+
 /* Returns n for a match, returns -1 for no match */
 
 int
@@ -395,6 +438,7 @@
 	Pixmap cPixmap;
 	decor_extents_t cFrame, cBorder, cMax_frame, cMax_border;
 	int cMin_width, cMin_height;
+	int q;
 	unsigned int cFrame_type, cFrame_state, cFrame_actions, cNQuad;
 	decor_quad_t cQuad[N_QUADS_MAX];
 	cNQuad = decor_pixmap_property_to_quads (data, i, size, &cPixmap, &cFrame, &cBorder, &cMax_frame,
@@ -404,10 +448,10 @@
 	if (cPixmap != *pixmap)
 	    continue;
 
-	if (memcmp (&cFrame, frame, sizeof (decor_extents_t)) ||
-	    memcmp (&cBorder, border, sizeof (decor_extents_t)) ||
-	    memcmp (&cMax_frame, max_frame, sizeof (decor_extents_t)) ||
-	    memcmp (&cMax_border, max_border, sizeof (decor_extents_t)))
+	if (decor_extents_cmp (&cFrame, frame) ||
+	    decor_extents_cmp (&cBorder, border) ||
+	    decor_extents_cmp (&cMax_frame, max_frame) ||
+	    decor_extents_cmp (&cMax_border, max_border))
 	    continue;
 
 	if (cFrame_type != frame_type ||
@@ -420,7 +464,11 @@
 	if (cNQuad != n_quad)
 	    continue;
 
-	if (memcmp (cQuad, quad, sizeof (decor_quad_t) * n_quad))
+	q = 0;
+	while (q < n_quad && !decor_quad_cmp (&cQuad[q], &quad[q]))
+	    q++;
+
+	if (q < n_quad)
 	    continue;
 
 	return n;