On Wed, 2006-01-25 at 17:32 -0500, Alex Villacís Lasso wrote:
- if (This == NULL) {
ERR("mouse hook called with no current lock!\n");return 0;- }
- if (code != HC_ACTION) return CallNextHookEx( This->hook, code,
wparam, lparam );
You need to call CallNextHookEx in any case, otherwise it might break other hooks in the chain. Probably just add a check for This being NULL in the same place as the code checks for HC_ACTION.
Dmitry Timoshkov wrote:
On Wed, 2006-01-25 at 17:32 -0500, Alex Villacís Lasso wrote:
- if (This == NULL) {
ERR("mouse hook called with no current lock!\n");return 0;- }
- if (code != HC_ACTION) return CallNextHookEx( This->hook, code,
wparam, lparam );
You need to call CallNextHookEx in any case, otherwise it might break other hooks in the chain. Probably just add a check for This being NULL in the same place as the code checks for HC_ACTION.
The problem is that if (This == NULL), then This->hook cannot be evaluated (for CallNextHookEx) without generating a segmentation fault. This is the very situation the patch is trying to prevent.
Alex Villacís Lasso
On Wed, 2006-01-25 at 17:55 -0500, Alex Villacís Lasso wrote:
The problem is that if (This == NULL), then This->hook cannot be evaluated (for CallNextHookEx) without generating a segmentation fault. This is the very situation the patch is trying to prevent.
Sorry for not spotting it earlier. 'This' can't be NULL, that's an impossible situation. You need to debug it and find out why hook proc is being called without This->hook being set.
Dmitry Timoshkov wrote:
On Wed, 2006-01-25 at 17:55 -0500, Alex Villacís Lasso wrote:
The problem is that if (This == NULL), then This->hook cannot be evaluated (for CallNextHookEx) without generating a segmentation fault. This is the very situation the patch is trying to prevent.
Sorry for not spotting it earlier. 'This' can't be NULL, that's an impossible situation. You need to debug it and find out why hook proc is being called without This->hook being set.
I already did. The scenario is as follows: 1) Start Fate/Stay Night (current_lock starts as NULL) 2) Mouse gets acquired (current_lock becomes valid), and mouse function gets hooked 3) User selects About dialog. 3) About dialog attempts a second (nested) mouse acquire, which (before my patch) succeeds because there is no check on whether current_lock is already set. Again, current_lock is assigned (overwriting the previous value), and the mouse function gets hooked (again). Notice that the mouse function is now hooked twice. 4) User dismisses the About dialog. Second acquire is released. The current_lock pointer gets set to NULL, and the second instance of the mouse function gets unhooked, but the first instance (the one hooked at program startup) remains hooked, now with current_lock == NULL. 5) Mouse event happens. Function hook gets called, and tries to use the NULL pointer. Instant segmentation fault.
The easiest way out of this (the one the patch implements) is to disallow nested acquires. It is a little more complicated if DirectInput actually requires support for nested acquires (why? I have only one mouse...)
Alex Villacís Lasso
On Wed, 2006-01-25 at 18:19 -0500, Alex Villacís Lasso wrote:
The easiest way out of this (the one the patch implements) is to disallow nested acquires. It is a little more complicated if DirectInput actually requires support for nested acquires (why? I have only one mouse...)
But if you disallow nested acquires the check for This being NULL in the hook proc should not be needed, right?
Dmitry Timoshkov wrote:
On Wed, 2006-01-25 at 18:19 -0500, Alex Villacís Lasso wrote:
The easiest way out of this (the one the patch implements) is to disallow nested acquires. It is a little more complicated if DirectInput actually requires support for nested acquires (why? I have only one mouse...)
But if you disallow nested acquires the check for This being NULL in the hook proc should not be needed, right?
Good point. But it doesn't hurt either. This might be changed into an assertion, since that is what the hook expects. What do you think?
Alex Villacís Lasso
On Wed, 2006-01-25 at 19:11 -0500, Alex Villacís Lasso wrote:
But if you disallow nested acquires the check for This being NULL in the hook proc should not be needed, right?
Good point. But it doesn't hurt either. This might be changed into an assertion, since that is what the hook expects. What do you think?
Yes, I think an assert would be more appropriate.
-
Alex Villacís Lasso -
Dmitry Timoshkov