server: Avoid accessing free'd thread pointers.

[Eric Pouech]

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+