Re: server: Avoid accessing free'd thread pointers.
IMO this should be fixed by rewriting correctly the _SAFE version of the list macros, which are to blame here. I have this somewhere, I'll send it later on. 2006/11/10, Mike McCormack <mike(a)codeweavers.com>:
--- server/process.c | 28 ++++++++++++++++++++++++---- 1 files changed, 24 insertions(+), 4 deletions(-)
-- Eric Pouech
Eric Pouech wrote:
IMO this should be fixed by rewriting correctly the _SAFE version of the list macros, which are to blame here. I have this somewhere, I'll send it later on.
The only way to prevent the next pointer (or any other pointer in the thread list) from being free'd is to grab it. Since the LIST macros doesn't understand pointer grabbing, I don't see a way to fix it by changing the macro... Mike
IIRC, the issue in this code is that you access in _SAFE macro the next field in the current cursor *after* the current cursor has been freed the issue is not that the next item has been freed while itering on the current cursor (this was at least the issue I had) so the first is to set the next cursor before entering the for loop and not after the execution of the for loop A+ 2006/11/10, Mike McCormack <mike(a)codeweavers.com>:
Eric Pouech wrote:
IMO this should be fixed by rewriting correctly the _SAFE version of the list macros, which are to blame here. I have this somewhere, I'll send it later on.
The only way to prevent the next pointer (or any other pointer in the thread list) from being free'd is to grab it. Since the LIST macros doesn't understand pointer grabbing, I don't see a way to fix it by changing the macro...
Mike
-- Eric Pouech
Eric Pouech wrote:
IIRC, the issue in this code is that you access in _SAFE macro the next field in the current cursor *after* the current cursor has been freed the issue is not that the next item has been freed while itering on the current cursor (this was at least the issue I had)
It looks like kill_thread can recurse if another thread is waiting on the current thread we're killing. wake_up -> wake_thread -> send_thread_wakeup -> kill_thread If the waiting thread is in the current process, and it's later in the list, I'm not sure anything stops it from being free'd. Mike
2006/11/10, Mike McCormack <mike(a)codeweavers.com>:
Eric Pouech wrote:
IIRC, the issue in this code is that you access in _SAFE macro the next field in the current cursor *after* the current cursor has been freed the issue is not that the next item has been freed while itering on the current cursor (this was at least the issue I had)
It looks like kill_thread can recurse if another thread is waiting on the current thread we're killing.
wake_up -> wake_thread -> send_thread_wakeup -> kill_thread
If the waiting thread is in the current process, and it's later in the list, I'm not sure anything stops it from being free'd.
well, the kill_thread in that case in only done when the waiting thread also died while waiting (ie has been killed by some other way) (in normal cases, the wait operation on the waiting side would just return an error code) the I'm not still conviced this path is actually executed in that case what lead you to write the patch ? A+ -- Eric Pouech
Eric Pouech wrote:
well, the kill_thread in that case in only done when the waiting thread also died while waiting (ie has been killed by some other way) (in normal cases, the wait operation on the waiting side would just return an error code) the I'm not still conviced this path is actually executed in that case
When the process is terminating because the user pressed ^C, the waiting thread will already be dead, so send_thread_wakeup will kill it.
what lead you to write the patch ?
valgrind reported that wineserver accessed free'd memory. On IRC, Alexandre suggested the fix is to change kill_process() to keep killing the first thread in the list until there's no more threads. Mike
Mike McCormack wrote:
Eric Pouech wrote:
well, the kill_thread in that case in only done when the waiting thread also died while waiting (ie has been killed by some other way) (in normal cases, the wait operation on the waiting side would just return an error code) the I'm not still conviced this path is actually executed in that case
When the process is terminating because the user pressed ^C, the waiting thread will already be dead, so send_thread_wakeup will kill it.
maybe, depending on the order of events
what lead you to write the patch ?
valgrind reported that wineserver accessed free'd memory.
I've seen it too, but it was only for current cursor not next, which still means we have : - the issue in process termination, where an iteration in the loop can kill more than one item in the list - the generic issue in the SAFE version, where we still access the cursor after it has been potentially fixed hence, the first topic is addressed by your patch, or the point below
On IRC, Alexandre suggested the fix is to change kill_process() to keep killing the first thread in the list until there's no more threads.
the second topic can be fixed with something along those lines diff --git a/include/wine/list.h b/include/wine/list.h index 6ceef8c..82ceb82 100644 --- a/include/wine/list.h +++ b/include/wine/list.h @@ -146,8 +146,8 @@ #define LIST_FOR_EACH(cursor,list) \ /* iterate through the list, with safety against removal */ #define LIST_FOR_EACH_SAFE(cursor, cursor2, list) \ for ((cursor) = (list)->next, (cursor2) = (cursor)->next; \ - (cursor) != (list); \ - (cursor) = (cursor2), (cursor2) = (cursor)->next) + (cursor) != (list) && ((cursor2) = (cursor)->next); \ + (cursor) = (cursor2)) /* iterate through the list using a list entry */ #define LIST_FOR_EACH_ENTRY(elem, list, type, field) \ A+
Eric Pouech <eric.pouech(a)wanadoo.fr> writes:
I've seen it too, but it was only for current cursor not next, which still means we have : - the issue in process termination, where an iteration in the loop can kill more than one item in the list
This is fixed now.
- the generic issue in the SAFE version, where we still access the cursor after it has been potentially fixed
No, in the SAFE macros we don't, that's the whole point of having them. -- Alexandre Julliard julliard(a)winehq.org
- the generic issue in the SAFE version, where we still access the cursor after it has been potentially fixed
No, in the SAFE macros we don't, that's the whole point of having them.
reading from wine/list.h #define LIST_FOR_EACH_SAFE(cursor, cursor2, list) \ for ((cursor) = (list)->next, (cursor2) = (cursor)->next; \ (cursor) != (list); \ (cursor) = (cursor2), (cursor2) = (cursor)->next) well, to me, the "(cursor2) = (cursor)->next" accesses cursor *AFTER* it has been potentially freed A+
Eric Pouech <eric.pouech(a)wanadoo.fr> writes:
reading from wine/list.h #define LIST_FOR_EACH_SAFE(cursor, cursor2, list) \ for ((cursor) = (list)->next, (cursor2) = (cursor)->next; \ (cursor) != (list); \ (cursor) = (cursor2), (cursor2) = (cursor)->next)
well, to me, the "(cursor2) = (cursor)->next" accesses cursor *AFTER* it has been potentially freed
No, cursor has just been set to cursor2. -- Alexandre Julliard julliard(a)winehq.org
Alexandre Julliard wrote:
Eric Pouech <eric.pouech(a)wanadoo.fr> writes:
reading from wine/list.h #define LIST_FOR_EACH_SAFE(cursor, cursor2, list) \ for ((cursor) = (list)->next, (cursor2) = (cursor)->next; \ (cursor) != (list); \ (cursor) = (cursor2), (cursor2) = (cursor)->next)
well, to me, the "(cursor2) = (cursor)->next" accesses cursor *AFTER* it has been potentially freed
No, cursor has just been set to cursor2.
/me scanning yellow pages for the nearest paper brown store A+
participants (4)
-
Alexandre Julliard -
Eric Pouech -
Eric Pouech -
Mike McCormack