a) When unloading conditional effects in joy.cpl, the program crashes: When unloading the conditional effect array entries, Wine tries to access an invalid (high) address because dieffect.lpvTypeSpecificParams was not set at the time the effect was created. So declare a valid 'standard' (maxed out) conditional effect. For other unknown effects: - The same can happen when CustomForce effects are available, but because Wine's FF dinput->linux translation does not support this yet, we simply print a FIXME to indicate a possible cause if a crash would happen. - The same for other (unsupported) effects (this is probably not safe?), but then we print a WARN. => This is probably not safe, but these effects won't be unloaded, because they will never be allowed to be created: I tested this by using a dummy FF device that advertised to support CustomForce, but because JoystickAImpl_EnumEffects() does not handle CustomForce, no CustomForce entry was generated in the joy.cpl application, and there was no crash at exit.
Solution: After the code of joy.cpl/main.c:766 : else if (IsEqualGUID(&pdei->guid, &GUID_Sine) || IsEqualGUID(&pdei->guid, &GUID_Square) || IsEqualGUID(&pdei->guid, &GUID_Triangle) || IsEqualGUID(&pdei->guid, &GUID_SawtoothUp) || IsEqualGUID(&pdei->guid, &GUID_SawtoothDown)) { DIPERIODIC pforce;
pforce.dwMagnitude = DI_FFNOMINALMAX; pforce.lOffset = 0; pforce.dwPhase = 0; pforce.dwPeriod = FF_PERIOD_TIME;
dieffect.cbTypeSpecificParams = sizeof(pforce); dieffect.lpvTypeSpecificParams = &pforce; dieffect.dwFlags |= DIEP_TYPESPECIFICPARAMS; } add the following code : else if (IsEqualGUID(&pdei->guid, &GUID_Spring) || IsEqualGUID(&pdei->guid, &GUID_Damper) || IsEqualGUID(&pdei->guid, &GUID_Inertia) || IsEqualGUID(&pdei->guid, &GUID_Friction)) { DICONDITION cxyforce[2];
int i; for (i = 0; i < 2; ++i) { cxyforce[i].lOffset = 0; cxyforce[i].lPositiveCoefficient = DI_FFNOMINALMAX; cxyforce[i].lNegativeCoefficient = DI_FFNOMINALMAX; cxyforce[i].dwPositiveSaturation = DI_FFNOMINALMAX; cxyforce[i].dwNegativeSaturation = DI_FFNOMINALMAX; cxyforce[i].lDeadBand = 0; }
dieffect.cbTypeSpecificParams = sizeof(cxyforce); dieffect.lpvTypeSpecificParams = cxyforce; dieffect.dwFlags |= DIEP_TYPESPECIFICPARAMS; } else if (IsEqualGUID(&pdei->guid, &GUID_CustomForce)) { FIXME("Unhandled type specific parameters of CustomForce effect, this may lead to memory errors later on.\n"); } else WARN("Possibly unhandled type specific parameters of effect with guid %x.\n", pdei->guid);
b) Weirdly, I had to add these commands that evaluates the DIEFFECT pointers at the end of each if-case, otherwise the pointers seemed to point to other address or data on that address was getting overwritten: TRACE("&rforce=0x%x\n", &rforce); TRACE("&cforce=0x%x\n", &cforce); TRACE("&pforce=0x%x\n", &pforce); TRACE("&cxyforce[0]=0x%x; &cxyforce[1]=0x%x\n", &(cxyforce[0]), &(cxyforce[1]));
Elias
On Tue, Mar 04, 2014 at 08:19:16PM +0100, Elias Vanderstuyft wrote:
a) When unloading conditional effects in joy.cpl, the program crashes: When unloading the conditional effect array entries, Wine tries to access an invalid (high) address because dieffect.lpvTypeSpecificParams was not set at the time the effect was created. So declare a valid 'standard' (maxed out) conditional effect. For other unknown effects: - The same can happen when CustomForce effects are available, but because Wine's FF dinput->linux translation does not support this yet, we simply print a FIXME to indicate a possible cause if a crash would happen. - The same for other (unsupported) effects (this is probably not safe?), but then we print a WARN.
For unhandled types, it's probably best to print a FIXME and return DIENUM_CONTINUE, rather than continue to rely on uninitialized data. Additionally, our IDirectInputEffect implementations could check for NULL lpvTypeSpecificParams and fail appropriately instead of crashing, if this is what Windows does.
b) Weirdly, I had to add these commands that evaluates the DIEFFECT pointers at the end of each if-case, otherwise the pointers seemed to point to other address or data on that address was getting overwritten: TRACE("&rforce=0x%x\n", &rforce); TRACE("&cforce=0x%x\n", &cforce); TRACE("&pforce=0x%x\n", &pforce); TRACE("&cxyforce[0]=0x%x; &cxyforce[1]=0x%x\n", &(cxyforce[0]), &(cxyforce[1]));
This is an out of scope problem. pforce, rforce, etc are local to the body of the if-blocks and fall out of scope when the if-blocks close. They should be moved to the top of the function.
By the way, when you begin to send patches to wine-patches, please only send up to 2-4 at a time, in logical groups. That way they can be reasonably reviewed & resubmitted as necessary.
Andrew
On Wed, Mar 5, 2014 at 3:49 PM, Andrew Eikum aeikum@codeweavers.com wrote:
On Tue, Mar 04, 2014 at 08:19:16PM +0100, Elias Vanderstuyft wrote:
a) When unloading conditional effects in joy.cpl, the program crashes: When unloading the conditional effect array entries, Wine tries to access an invalid (high) address because dieffect.lpvTypeSpecificParams was not set at the time the effect was created. So declare a valid 'standard' (maxed out) conditional effect. For other unknown effects: - The same can happen when CustomForce effects are available, but because Wine's FF dinput->linux translation does not support this yet, we simply print a FIXME to indicate a possible cause if a crash would happen. - The same for other (unsupported) effects (this is probably not safe?), but then we print a WARN.
For unhandled types, it's probably best to print a FIXME and return DIENUM_CONTINUE, rather than continue to rely on uninitialized data.
Alright. Should I also write the extra if-test for the CustomForce type, or do I have to cover it with one if-test as an unhandled type?
Additionally, our IDirectInputEffect implementations could check for NULL lpvTypeSpecificParams and fail appropriately instead of crashing, if this is what Windows does.
Well, I checked the SetParameters() function, and it does correctly check on NULL lpvTypeSpecificParams and fail appropriately. The problem seems again to be related with joy.cpl/main.c: there is no check performed on the result of IDirectInputDevice2_CreateEffect(), so change joy.cpl/main.c:784 : hr = IDirectInputDevice2_CreateEffect( joystick->device, &pdei->guid, &dieffect, &joystick->effects[joystick->cur_effect].effect, NULL);
joystick->effects[joystick->cur_effect].params = dieffect; joystick->effects[joystick->cur_effect].info = *pdei; joystick->cur_effect += 1;
return DIENUM_CONTINUE; to : hr = IDirectInputDevice2_CreateEffect( joystick->device, &pdei->guid, &dieffect, &joystick->effects[joystick->cur_effect].effect, NULL); if (FAILED(hr)) return DIENUM_CONTINUE;
joystick->effects[joystick->cur_effect].params = dieffect; joystick->effects[joystick->cur_effect].info = *pdei; joystick->cur_effect += 1;
return DIENUM_CONTINUE;
Before I knew the joy.cpl app was to be blamed, and was searching in the IDirectInputEffect implementions, I saw something that is not conform MSDN: JoystickAImpl_EnumEffects and JoystickWImpl_EnumEffects should check on DIENUM_CONTINUE to continue the enumeration, or DIENUM_STOP to stop it: http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sd... For example, in JoystickWImpl_EnumEffects(), each occurrence of "(*lpCallback)(&dei, pvRef);" should check whether the return value is DIENUM_CONTINUE or DIENUM_STOP. When the latter is the case, should we then first add a "goto" statement to jump to the last "if (xfd == -1) IDirectInputDevice8_Unacquire(iface);" statement, or directly return? And what should it return in that case, "DIERR_NOTINITIALIZED" (does stopping enumeration mean failure? that's not clear in MSDN), or "DI_OK"? And even worse, what if the return value of "(*lpCallback)(&dei, pvRef);" is not equal to DIENUM_CONTINUE or DIENUM_STOP? Should JoystickWImpl_EnumEffects() interpret it as the former or as the latter? (also not mentioned in MSDN)
b) Weirdly, I had to add these commands that evaluates the DIEFFECT pointers at the end of each if-case, otherwise the pointers seemed to point to other address or data on that address was getting overwritten: TRACE("&rforce=0x%x\n", &rforce); TRACE("&cforce=0x%x\n", &cforce); TRACE("&pforce=0x%x\n", &pforce); TRACE("&cxyforce[0]=0x%x; &cxyforce[1]=0x%x\n", &(cxyforce[0]), &(cxyforce[1]));
This is an out of scope problem. pforce, rforce, etc are local to the body of the if-blocks and fall out of scope when the if-blocks close. They should be moved to the top of the function.
Ah, interesting, thanks! I'll move their declarations to the top of the function in the corresponding patch that I will send. To clarify for other people: Change joy.cpl/main.c:741 : dieffect.cAxes = 2; dieffect.rgdwAxes = axes; dieffect.rglDirection = direction;
if (IsEqualGUID(&pdei->guid, &GUID_RampForce)) to : dieffect.cAxes = 2; dieffect.rgdwAxes = axes; dieffect.rglDirection = direction;
DIRAMPFORCE rforce; DICONSTANTFORCE cforce; DIPERIODIC pforce; DICONDITION cxyforce[2];
if (IsEqualGUID(&pdei->guid, &GUID_RampForce))
By the way, when you begin to send patches to wine-patches, please only send up to 2-4 at a time, in logical groups. That way they can be reasonably reviewed & resubmitted as necessary.
OK, no problem ;)
Elias
On Fri, Mar 07, 2014 at 10:01:30PM +0100, Elias Vanderstuyft wrote:
On Wed, Mar 5, 2014 at 3:49 PM, Andrew Eikum aeikum@codeweavers.com wrote:
On Tue, Mar 04, 2014 at 08:19:16PM +0100, Elias Vanderstuyft wrote:
a) When unloading conditional effects in joy.cpl, the program crashes: When unloading the conditional effect array entries, Wine tries to access an invalid (high) address because dieffect.lpvTypeSpecificParams was not set at the time the effect was created. So declare a valid 'standard' (maxed out) conditional effect. For other unknown effects: - The same can happen when CustomForce effects are available, but because Wine's FF dinput->linux translation does not support this yet, we simply print a FIXME to indicate a possible cause if a crash would happen. - The same for other (unsupported) effects (this is probably not safe?), but then we print a WARN.
For unhandled types, it's probably best to print a FIXME and return DIENUM_CONTINUE, rather than continue to rely on uninitialized data.
Alright. Should I also write the extra if-test for the CustomForce type, or do I have to cover it with one if-test as an unhandled type?
I'd just treat it as an unhandled type.
Additionally, our IDirectInputEffect implementations could check for NULL lpvTypeSpecificParams and fail appropriately instead of crashing, if this is what Windows does.
Well, I checked the SetParameters() function, and it does correctly check on NULL lpvTypeSpecificParams and fail appropriately. The problem seems again to be related with joy.cpl/main.c: there is no check performed on the result of IDirectInputDevice2_CreateEffect(), so change joy.cpl/main.c:784 : hr = IDirectInputDevice2_CreateEffect( joystick->device, &pdei->guid, &dieffect, &joystick->effects[joystick->cur_effect].effect, NULL);
joystick->effects[joystick->cur_effect].params = dieffect; joystick->effects[joystick->cur_effect].info = *pdei; joystick->cur_effect += 1; return DIENUM_CONTINUE;to : hr = IDirectInputDevice2_CreateEffect( joystick->device, &pdei->guid, &dieffect, &joystick->effects[joystick->cur_effect].effect, NULL); if (FAILED(hr)) return DIENUM_CONTINUE;
joystick->effects[joystick->cur_effect].params = dieffect; joystick->effects[joystick->cur_effect].info = *pdei; joystick->cur_effect += 1; return DIENUM_CONTINUE;
Sure.
Before I knew the joy.cpl app was to be blamed, and was searching in the IDirectInputEffect implementions, I saw something that is not conform MSDN: JoystickAImpl_EnumEffects and JoystickWImpl_EnumEffects should check on DIENUM_CONTINUE to continue the enumeration, or DIENUM_STOP to stop it: http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sd... For example, in JoystickWImpl_EnumEffects(), each occurrence of "(*lpCallback)(&dei, pvRef);" should check whether the return value is DIENUM_CONTINUE or DIENUM_STOP. When the latter is the case, should we then first add a "goto" statement to jump to the last "if (xfd == -1) IDirectInputDevice8_Unacquire(iface);" statement, or directly return? And what should it return in that case, "DIERR_NOTINITIALIZED" (does stopping enumeration mean failure? that's not clear in MSDN), or "DI_OK"?
Probably the goto. I'm not sure about the return value, you would need tests to be certain. Probably DI_OK is correct.
And even worse, what if the return value of "(*lpCallback)(&dei, pvRef);" is not equal to DIENUM_CONTINUE or DIENUM_STOP? Should JoystickWImpl_EnumEffects() interpret it as the former or as the latter? (also not mentioned in MSDN)
Again, you'd need tests to confirm.
dieffect.cAxes = 2; dieffect.rgdwAxes = axes; dieffect.rglDirection = direction; DIRAMPFORCE rforce; DICONSTANTFORCE cforce; DIPERIODIC pforce; DICONDITION cxyforce[2]; if (IsEqualGUID(&pdei->guid, &GUID_RampForce))
Yes, but the declarations have to be at the very top of the function, alongside all of the other declarations.
Andrew
On Mon, Mar 10, 2014 at 4:47 PM, Andrew Eikum aeikum@codeweavers.com wrote:
On Fri, Mar 07, 2014 at 10:01:30PM +0100, Elias Vanderstuyft wrote:
On Wed, Mar 5, 2014 at 3:49 PM, Andrew Eikum aeikum@codeweavers.com wrote:
On Tue, Mar 04, 2014 at 08:19:16PM +0100, Elias Vanderstuyft wrote:
a) When unloading conditional effects in joy.cpl, the program crashes: When unloading the conditional effect array entries, Wine tries to access an invalid (high) address because dieffect.lpvTypeSpecificParams was not set at the time the effect was created. So declare a valid 'standard' (maxed out) conditional effect. For other unknown effects: - The same can happen when CustomForce effects are available, but because Wine's FF dinput->linux translation does not support this yet, we simply print a FIXME to indicate a possible cause if a crash would happen. - The same for other (unsupported) effects (this is probably not safe?), but then we print a WARN.
For unhandled types, it's probably best to print a FIXME and return DIENUM_CONTINUE, rather than continue to rely on uninitialized data.
Alright. Should I also write the extra if-test for the CustomForce type, or do I have to cover it with one if-test as an unhandled type?
I'd just treat it as an unhandled type.
OK.
Before I knew the joy.cpl app was to be blamed, and was searching in the IDirectInputEffect implementions, I saw something that is not conform MSDN: JoystickAImpl_EnumEffects and JoystickWImpl_EnumEffects should check on DIENUM_CONTINUE to continue the enumeration, or DIENUM_STOP to stop it: http://msdn.microsoft.com/en-us/library/windows/desktop/microsoft.directx_sd... For example, in JoystickWImpl_EnumEffects(), each occurrence of "(*lpCallback)(&dei, pvRef);" should check whether the return value is DIENUM_CONTINUE or DIENUM_STOP. When the latter is the case, should we then first add a "goto" statement to jump to the last "if (xfd == -1) IDirectInputDevice8_Unacquire(iface);" statement, or directly return? And what should it return in that case, "DIERR_NOTINITIALIZED" (does stopping enumeration mean failure? that's not clear in MSDN), or "DI_OK"?
Probably the goto. I'm not sure about the return value, you would need tests to be certain. Probably DI_OK is correct.
OK, I'll queue all patches that need testing (and open a bug report if I really don't have enough time to create the tests by myself.) For the other patches, I'll post them as soon as possible to wine-patches.
And even worse, what if the return value of "(*lpCallback)(&dei, pvRef);" is not equal to DIENUM_CONTINUE or DIENUM_STOP? Should JoystickWImpl_EnumEffects() interpret it as the former or as the latter? (also not mentioned in MSDN)
Again, you'd need tests to confirm.
OK.
dieffect.cAxes = 2; dieffect.rgdwAxes = axes; dieffect.rglDirection = direction; DIRAMPFORCE rforce; DICONSTANTFORCE cforce; DIPERIODIC pforce; DICONDITION cxyforce[2]; if (IsEqualGUID(&pdei->guid, &GUID_RampForce))Yes, but the declarations have to be at the very top of the function, alongside all of the other declarations.
Oh, thanks for noticing, I should have known that ;)
Elias
Hi,
I've got an additional question about EnumEffects(): Why does the JoystickWImpl_EnumEffects() variant add the additional code (in contrast with the 'A' function variant)? : int xfd = This->joyfd; -- snip -- /* return to unacquired state if that's where it was */ if (xfd == -1) IDirectInputDevice8_Unacquire(iface);
Elias
-
Andrew Eikum -
Elias Vanderstuyft