[Xcb] [PATCH] memcpy() with wrong size in xcb/util/icccm
pharris at opentext.com
Mon Dec 21 12:10:44 PST 2009
Michael Stapelberg wrote:
> The problem seems to be using the wrong length for memcpy in
> xcb_get_wm_size_hints_from_reply(), see this patch:
> --- icccm/O.icccm.c 2009-12-21 19:33:35.019052049 +0100
> +++ icccm/icccm.c 2009-12-21 19:33:42.951051864 +0100
> @@ -445,8 +445,7 @@
> length >= 15))
> return 0;
> - memcpy(hints, (xcb_size_hints_t *) xcb_get_property_value (reply),
> - length * reply->format >> 3);
> + memcpy(hints, (xcb_size_hints_t *) xcb_get_property_value (reply), length);
> flags = (XCB_SIZE_HINT_US_POSITION | XCB_SIZE_HINT_US_SIZE |
> XCB_SIZE_HINT_P_POSITION | XCB_SIZE_HINT_P_SIZE |
Well, 'length' is divided by reply->format/8 just a few lines above, so
it needs to be multiplied by reply->format/8 again to get the number of
> I think this is the correct way to do it because it matches the
Are you sure?
> and because xcb_get_wm_hints_from_reply() also does
> not multiply with (reply->format >> 3).
xcb_get_wm_hints_from_reply() does not divide by reply->format/8, which
is why it doesn't need to multiply by it again. (Well, it does, but it
calls the result num_elem).
> Still, it would be great if anyone of
> you more familiar with this code could review it and apply the patch for the
> next release of xcb-util.
There is definitely a major problem with this code: It copies an
untrusted amount of data into a fixed length buffer. See the attached
patch; does this fix the problem for you?
To the list at large (since this has been present since before the
switch to git): is there a reason WM_SIZE_HINTS was allowed to have a
format other than 32? Other formats are broken in a mixed-endianness
environment, so I can't imagine the spec ever allowed them.
Open Text Connectivity Solutions Group
Peter Harris http://connectivity.opentext.com/
Research and Development Phone: +1 905 762 6001
pharris at opentext.com Toll Free: 1 877 359 4866
More information about the Xcb