[rdesktop-devel] [ rdesktop-Patches-1970489 ] NET_WM_ICON segfault on x86_64

[SourceForge.net <no...@so...>]

Patches item #1970489, was opened at 2008-05-23 14:41
Message generated for change (Comment added) made by ribenakid
You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=381349&aid=1970489&group_id=24366

Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: None
Group: None
Status: Open
Resolution: None
Priority: 5
Private: No
Submitted By: Nobody/Anonymous (nobody)
Assigned to: Nobody/Anonymous (nobody)
Summary: NET_WM_ICON segfault on x86_64

Initial Comment:
On x86_64 using 1.6.0 and seamlessrdp I get a segfault in ewmh_set_icon and ewmh_del_icon.  To reproduce: use rdesktop to seamlessrdp load IE on XP SP2, start browsing, boom.

http://2006.planet-soc.com/node/265/ explains why fault occurred (same problem, different project):

"Problem was in X functions XChangeProperty and XGetWindowProperty. They are used to set/retrieve some additional information associated with X windows. One of their arguments is 'format', which (on 32bit machines) says the size of one 'item' stored in property. The functions then expect array of these items. But, there is an important phrase in manpage, which was overlooked: " If the returned format is 32, the returned data is represented as a long array...". And the important thing is, that on x86_64, size of long is not 32, but 64 bits. The function expects array of 'long's, from which it always takes lower 32bit and discards the rest. So the resulted data which were stored and consecutively retrieved with old method, missed every odd 'int'. And, with data damaged this way, rdesktop could crash."

Patch to fix this is attached - changes using uint32s in a few places to unsigned longs.  Tested and fixed my x86_64 box, should be a no-op on i386...

Cheers,

Jim

----------------------------------------------------------------------

Comment By: TRK (ribenakid)
Date: 2009-10-14 13:35

Message:
Bug #1967935 deals with this issue too but suggests changes to make the
code more robust in the face of corrupt NET_WM_ICON data.

I'd also query the pointer arithmetic in the memcpy on line 524. Adding i
to the new_set pointer will add i*sizeof(*new_set) bytes onto the value of
the new_set pointer, i.e. the compiler is already multiplying i by
sizeof(unsigned long) so I don't think it should be being done again in the
code.

The purpose of the memcpy seems to be to remove the icon at offset i of
size icon_size by copying the data after that icon into the new set.
Therefore I think the memcpy should actually be:

memcpy(new_set + i, cur_set + i + icon_size, (nitems - (i +
icon_size))*sizeof(unsigned long));

This also tallies better with the parameters used in the memcpy at line
522.


----------------------------------------------------------------------

Comment By: Tristan Miller (psychonaut25)
Date: 2009-10-06 11:50

Message:
This patch also fixes Bug 2168354, so I've marked it as a duplicate.

----------------------------------------------------------------------

Comment By: ChD (chd-fr)
Date: 2009-10-05 10:38

Message:
Very useful patch while using seamlessrdp on x86_64. Otherwise, rdesktop
crashes each time a window is opened or closed.
So, like said yeroc : "What needs to be done to get this patch applied to
version control so this gets fixed in an official release?"

----------------------------------------------------------------------

Comment By: Corey Puffalt (yeroc)
Date: 2009-09-24 22:16

Message:
What needs to be done to get this patch applied to version control so this
gets fixed in an official release?

----------------------------------------------------------------------

You can respond by visiting: 
https://sourceforge.net/tracker/?func=detail&atid=381349&aid=1970489&group_id=24366