race condition in dinput's multiple joystick code and IL2

[Christoph Frick]

hi there,

the code i submitted some weeks ago about having more than one
/dev/input/event-joystick shows from time to time a race condition in
IL2 Sturmovik. this game is written in java and somehow exe-fied. so the
whole thing is running a boatload of threads. my box is a
smp-opteron-system with gentoo and a 2.6.17 kernel.

my problem is the following (in dlls/dinput/joystick_linuxinput.c): the
new code calls a method (find_joydevs) on serveral places which checks
by a global variable, whether it ran before (have_joydevs) and then
builds up a list of the valid devices (joydevs). so what happes there is
a struct is filled on the stack and later memcpy-ied to the end of a
(re)alloced (HeapAlloc/HeapRealloc) list. things like the device are
strdup'ed from the stack into the struct before copy.

so in IL2 often - but not always - the have_joydevs has a count > -1,
the joydevs has a pointer. but when it access the "struct" behind it,
this is uninitialized memory (at least for the device-member) and random
errors occur.

so what i have tried so far:
- put find_joydevs into a critical section
- dont use strdup, but HeapAlloc+lstrcpyA

nothing helped. the only other thing that came this morning into my mind
but i have not yet tried to change, is the fact, that i malloc the
string and store the pointer to it in the struct. next thing is a memcpy
of the whole struct, which seems ok to my c-knowledge.

so if one of the wine gurus has some tips i would be glad to try them
out.

-- 
cu

[Robert Shearman]

Christoph Frick wrote:
> hi there,
>
> the code i submitted some weeks ago about having more than one
> /dev/input/event-joystick shows from time to time a race condition in
> IL2 Sturmovik. this game is written in java and somehow exe-fied. so the
> whole thing is running a boatload of threads. my box is a
> smp-opteron-system with gentoo and a 2.6.17 kernel.
>
> my problem is the following (in dlls/dinput/joystick_linuxinput.c): the
> new code calls a method (find_joydevs) on serveral places which checks
> by a global variable, whether it ran before (have_joydevs) and then
> builds up a list of the valid devices (joydevs). so what happes there is
> a struct is filled on the stack and later memcpy-ied to the end of a
> (re)alloced (HeapAlloc/HeapRealloc) list. things like the device are
> strdup'ed from the stack into the struct before copy.
>
> so in IL2 often - but not always - the have_joydevs has a count > -1,
> the joydevs has a pointer. but when it access the "struct" behind it,
> this is uninitialized memory (at least for the device-member) and random
> errors occur.
>
> so what i have tried so far:
> - put find_joydevs into a critical section

This should work. Why don't you share the patch you used to do this? 
Note that none of the data in JoystickImpl is protected by that critical 
section and maybe it should be.

-- 
Rob Shearman

[Christoph Frick]

On Wed, Jul 19, 2006 at 02:03:06PM +0100, Robert Shearman wrote:

Hi Robert,

thanks for your comments.
> > so what i have tried so far:
> > - put find_joydevs into a critical section
> > This should work. Why don't you share the patch you used to do this?

because i have dumped it, after there where no success ;)

it was basically like this: put a crititcal section and the _debug stuff
(stolen from another dll) in the file and call Enter/Leave on the
start/end of the find_joydevs method.
> Note that none of the data in JoystickImpl is protected by that
> critical section and maybe it should be.

in general i dont really like the idea of calling find_joydevs every
time it *might* be needed and i would prefer to go a way like
registering also hooks in the dinput-implementations to allow the
devices to "find themself" on dinput.dll's startup (add a init-method
where the enum/create hooks are). this is what i would have tried next
(and i guess still will). there i can put a critical secion also.

-- 
cu

[H. Verbeet]

I didn't look at the threading issue, but there is at least one part
of that patch that looks wrong to me:

In joydev_enum_deviceW():
> -  if (id != 0)
> -      return FALSE;
> +  if (id >= have_joydevs) {
> +    return -1;
> +  }
> I think that should return FALSE rather than -1 (TRUE).