Update MinHook to 1.3.3#8142
Conversation
|
Hi, any interesting things to look at based on changelog? Thanks.
From: Leonard de Ruijter <notifications@github.com>
Sent: Wednesday, April 4, 2018 8:34 AM
To: nvaccess/nvda <nvda@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Subject: [nvaccess/nvda] Update MinHook to 1.3.3 (#8142)
Link to issue number:
None
Summary of the issue:
MinHook, the hooking library used to hook into processes, has been on 1.2.2 for years within NVDA. Version 1.3.3 is the most recent version.
Description of how this pull request fixes the issue:
This updates MinHook to 1.3.3 and updates the associated sconscript file as well.
Testing performed:
Try build ran without any observed issues, but wider testing would be appreciated.
Known issues with pull request:
None
…_____
You can view, comment on, or merge this pull request online at:
#8142
Commit Summary
* Update minhook to 1.3.3
* Fix log for MH_Initialize
* Updated license for minhook
File Changes
* M copying.txt <https://github.com/nvaccess/nvda/pull/8142/files#diff-0> (150)
* M include/minhook <https://github.com/nvaccess/nvda/pull/8142/files#diff-1> (2)
* M nvdaHelper/minHook/sconscript <https://github.com/nvaccess/nvda/pull/8142/files#diff-2> (10)
* M nvdaHelper/remote/apiHook.cpp <https://github.com/nvaccess/nvda/pull/8142/files#diff-3> (2)
Patch Links:
* https://github.com/nvaccess/nvda/pull/8142.patch
* https://github.com/nvaccess/nvda/pull/8142.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#8142> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AHgLkIpJYctL7zdHne_PQu5LMSXJJ3IAks5tlOgBgaJpZM4THBbX> .
|
|
Here are the relevant changes that might have impact:
|
…. Provide more user friendly logging with StatusToString
|
As it now stands, it looks like this pr doesn't allow my Edge to be started any more as soon as the try build is installed. :) |
Can you confirm: this PR breaks Edge? Can you elaborate on what happens? |
| Most of the source code for NVDA itself is available under this license. | ||
|
|
||
| ``` | ||
| `` |
There was a problem hiding this comment.
This should probably still be 3 ` characters
|
@feerrenrut commented on 9 apr. 2018 10:26 CEST:
On one of my systems, as soon as I run this branch as an installed copy, Edge does seem to close around 2 seconds after I start it. When running as a portbale copy, this problem does not occur. I have already tracked down that this is most likely cause by the more friendly error logging I tried to implement. Without these changes to error logging, A try build functions correctly. I'm currently investigating how to fix this. |
…ing the module and function names in the error
|
Error logging is now again working as expected. |
| void* origFunc; | ||
| int res; | ||
| MH_STATUS res; | ||
| if((res=MH_CreateHook_fp(realFunc,newHookProc,&origFunc))!=MH_OK) { |
There was a problem hiding this comment.
assignments done inside if blocks can easily become a source of error. I would much prefer to see this changed to:
MH_STATUS res = MH_CreateHook_fp(realFunc, newHookProc, &origFunc);
if( res != MH_OK) {
This gets rid of the declared but not initialised res, and reduces the logic inside the if statement.
|
Thanks for pointing this out. I've also addressed similar uses of res
variables to maintain consistency.
|
|
On Windows 7 32bit with MinHook 1.3.3 there are great problems with UAC. |
|
Thanks for your report. Could you please file a new issue for this,
along with steps to reproduce?
|
Link to issue number:
None
Summary of the issue:
MinHook, the hooking library used to hook into processes, has been on 1.2.2 for years within NVDA. Version 1.3.3 is the most recent version.
Description of how this pull request fixes the issue:
This updates MinHook to 1.3.3 and updates the associated sconscript file as well.
Testing performed:
Try build ran without any observed issues, but wider testing would be appreciated.
Known issues with pull request:
None