New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

other apps may crash or cause errors when an NVDA installation is updated #7563

Closed
michaelDCurran opened this Issue Sep 4, 2017 · 8 comments

Comments

Projects
None yet
5 participants
@michaelDCurran
Contributor

michaelDCurran commented Sep 4, 2017

Steps to reproduce:

  1. Install NVDA 2017.3 and reboot.
  2. With NVDA running, open apps such as Firefox or Mumble.
  3. Install this NVDA Next snapshot
  4. Once NVDA has installed and restarted itself, Start alt+tabbing around the system.

Expected behavior:

The apps should not crash.

Actual behavior:

One or more of the apps crash.

System configuration:

Windows version: 10.0.16278 64-bit

Technical info:

We forget to unhookWinEvent in NVDAHelperRemote

NVDAHelperRemote injects into applications with foreground and focus winEvents. In inprocMgrThreadFunc, it then registers another winEvent limited to that process (inproc_winEventCallback) that listens for all possible winEvents. On termination, the injection winEvent is unregistered, but the process-specific winEvent is not. This leaves NVDAHelperRemote.dll in the process for ever. It will also respond to winEvents even though nvdaHelperRemote is terminated which may cause instability or a crash. It is necessary that inproc_winEventCallback be unregistered before inprocMgrThreadFunc exits.

Accidentally execute invalid memory

It is possible to accidentally cause Windows to execute incorrect memory in an in-context winEvent hook and thereby crash the injected process, when using a valid dll and a valid executable function pointer from that dll.
Specifically:
Create 2 dlls. Each with a winEventCallback function. However, add extra code to dll1's dllMain so that it does an extra addref (GetModuleHandleEx) on process attach so that it never unloads.
Then:
A. Rename dll1 to tempPath
A. Load dll 1 via tempPath into process1
B. Call SetWinEventHook giving it the handle of dll1, and the function pointer of a WINEVENTCALLBACK within dll1, make it listen for events in process2, in-context.
C. Fire a winEvent in process2, so that Windows injects dll1 and executes the winEventCallback.
D. Unhook dll1's winEventCallback in process1
E. Rename dll2 to tempPath (replacing dll1)
F. Load dll2 via tempPath into process1
G. Call SetWinEventHook giving it the handle of dll2, and the function pointer of a WINEVENTCALLBACK within dll2, make it listen for events in process2, in-context.
H. Fire a winEvent in process2, so that Windows should inject dll2 and execute the winEventCallback.
However:
As dll1 and dll2 both have exactly the same path, and dll1 is still loaded, Windows does not bother loading dll2. Rather it uses dll1's memory address to calculate the physical location of the winEventCalback. But, as this was originally from dll2, it hits random memory!
In other words, It seems that the only information sent from process1 to process2 is the path of the dll, and the offset of the winEventCallback (I.e. winEventCallback minus dll base address). Then in process2, it does a simple LoadLibrary(path) and calculates dllHandle plus winEventCallback offset.

Practical outcome

Normally when updating an NVDA installation, as nvdaHelperRemote.dll did not change its layout, things go okay. However, if nvdaHelperRemote is changed in any dramatic way (such as a segment being removed or code moved around such as with the mentioned Next snapshot) then if the old nvdaHelperRemote takes a long time to unload from an app, and in deed it is still there when the new NVDA tries to inject, Windows will refuse to load in the new copy as it thinks it already has it, and then executes from the wrong memory location.
We specifically hit this problem when implementing pr #7535

Suggested fix

We should place all NVDA dlls in a version-specific directory, so that the path to any NVDA dll changes for each version.

@Brian1Gaff

This comment has been minimized.

Show comment
Hide comment
@Brian1Gaff

Brian1Gaff Sep 4, 2017

Brian1Gaff commented Sep 4, 2017

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Sep 4, 2017

Contributor
Contributor

michaelDCurran commented Sep 4, 2017

@beqabeqa473

This comment has been minimized.

Show comment
Hide comment
@beqabeqa473

beqabeqa473 Sep 4, 2017

hi.

why it is not possible to free library just before exiting nvda?

beqabeqa473 commented Sep 4, 2017

hi.

why it is not possible to free library just before exiting nvda?

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Sep 4, 2017

Contributor
Contributor

michaelDCurran commented Sep 4, 2017

@Brian1Gaff

This comment has been minimized.

Show comment
Hide comment
@Brian1Gaff

Brian1Gaff Sep 4, 2017

Brian1Gaff commented Sep 4, 2017

@michaelDCurran

This comment has been minimized.

Show comment
Hide comment
@michaelDCurran

michaelDCurran Sep 4, 2017

Contributor
Contributor

michaelDCurran commented Sep 4, 2017

@Brian1Gaff

This comment has been minimized.

Show comment
Hide comment
@Brian1Gaff

Brian1Gaff Sep 5, 2017

Brian1Gaff commented Sep 5, 2017

@fisher729

This comment has been minimized.

Show comment
Hide comment
@fisher729

fisher729 Nov 17, 2017

Hi. There's an I in 'compared' that shouldn't be there. :)

fisher729 commented Nov 17, 2017

Hi. There's an I in 'compared' that shouldn't be there. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment