Ensure that NVDA can get file version information for binaries in system32 on 64-bit versions of Windows #12943

lukaszgo1 · 2021-10-16T16:59:49Z

Link to issue number:

None, discussion in #12852

Summary of the issue:

NVDA can retrieve information for a binary file such as application name and version from which given appModule has been created. While they're not shown to the user directly they can be inspected as part of the developer info - also NVDA relies on them for various stuff internally. For binaries placed in system32 (such as LogonUI.exe) this process fails on 64-bit versions of Windows as follows:

  File "appModuleHandler.pyc", line 413, in _get_productName
  File "appModuleHandler.pyc", line 405, in _setProductInfo
  File "appModuleHandler.pyc", line 363, in _getExecutableFileInfo
  File "fileUtils.pyc", line 48, in getFileVersionInfo
RuntimeError: The file C:\Windows\System32\consent.exe does not exist

This occurs because NVDA is a 32-bit process and when trying to access C:\Windows\System32 Windows redirects it to C:\Windows\SysWOW64 and some binaries aren't duplicated there.

Description of how this pull request fixes the issue:

For a duration of retrieving file info for a binary placed in system32 and when running on a version of Windows which have both system32 and SysWOW64 WOW64 redirection is disabled. While at it I've removed deprecated shlobj.SHGetFolderPath and replaced it with SHGetKnownFolderPath and removed unused function from the config module ((while not strictly related it relied on SHGetFolderPath).

Testing strategy:

On 64-bit version of Windows installed self signed build from this branch, enabled service debug, started an application with the admin privileges, inspected developer info for the UAC screen and made sure that for consent.exe file version and file name can be retrieved. Made sure that this code is not executed on 32-bit version of Windows and that file version info can still be retrieved there.

Known issues with pull request:

Breaks backward compatibility - it is unlikely to be merged before 2022.1 so not a big deal IMO.

Change log entries:

Bug fixes

NVDA can report application name and version for binaries placed in system32 when running under 64-bit version of Windows.

For Developers

config.getSystemConfigPath has been removed - there is no replacement
shlobj.SHGetFolderPath has been removed - please use shlobj.SHGetKnownFolderPath instead

Code Review Checklist:

Pull Request description:
- description is up to date
- change log entries
Testing:
- Unit tests
- System (end to end) tests
- Manual testing
API is compatible with existing add-ons.
Documentation:
- User Documentation
- Developer / Technical Documentation
- Context sensitive help for GUI changes
UX of all users considered:
- Speech
- Braille
- Low Vision
- Different web browsers
- Localization in other languages / culture than English

…winKernel

- Clean start imports - Update copyright header - Use enums and add some new CSIDL constants

…n systes32 by temporarily suspending SysWOW64 redirection

lukaszgo1 · 2021-10-16T17:01:50Z

@LeonarddeR Given our discussion in #12852 you may be interested in reviewing this.

LeonarddeR · 2021-10-18T09:04:00Z

source/appModuleHandler.py

+from functools import wraps
+
+# Path to the native system32 directory.
+nativeSys32: str = shlobj.SHGetFolderPath(None, shlobj.CSIDL.SYSTEM)


Could we have this initialization as part of the initialize method?

I don't think this is a good idea here. I really believe we should try to avoid usages of global if at all possible - it makes any refactors unnecessarily difficult. My dislike for global aside please note that appmoduleHandler.initialize is also executed when re-initializing app modules and therefore we would be calling SHGetFolderPath for no reason ( these calls are probably pretty cheap but the return values cannot change during NVDA's lifetime). It should be possible to add additional argument to initialize - something like isFirstInit but it would make code more complex without any real benefit.

It might be my daily job being very C# oriented nowadays, but I consider executing code at the module level pretty terrible.
I don't insist though, let's see what NV Access people say.

I would agree that initializing this in a method would be much better.
If _suspendWow64RedirectionForFileInfoRetrieval could be useful in other modules, I would suggest abstracting this into a new module an initializing this in core to avoid the "re-initializing" you mention.

source/appModuleHandler.py

LeonarddeR · 2021-10-18T09:06:19Z

source/config/__init__.py

@@ -126,14 +128,6 @@ def getUserDefaultConfigPath(useInstalledPathIfExists=False):
 		return installedUserConfigPath
 	return os.path.join(globalVars.appDir, 'userConfig')

-def getSystemConfigPath():


This is likely to break backwards compat. I don't think that's a problem when this will be 2022.1

I've mentioned that consideration in the PR description. Given that we're branching for 2021.3 already this should not be a problem. Could you add this PR to the 2022.1 milestone so it is not forgotten?

thanks, I must have missed that.

source/shlobj.py

lukaszgo1 · 2021-10-19T08:59:24Z

Given development cycle for 2022.1 has began I'm marking this as ready for review.

seanbudd

Thanks @lukaszgo1

source/config/__init__.py

source/appModuleHandler.py

source/shlobj.py

seanbudd · 2021-10-19T23:44:50Z

source/appModuleHandler.py

+from functools import wraps
+
+# Path to the native system32 directory.
+nativeSys32: str = shlobj.SHGetFolderPath(None, shlobj.CSIDL.SYSTEM)


I would agree that initializing this in a method would be much better.
If _suspendWow64RedirectionForFileInfoRetrieval could be useful in other modules, I would suggest abstracting this into a new module an initializing this in core to avoid the "re-initializing" you mention.

source/appModuleHandler.py

michaelDCurran · 2021-10-20T06:35:31Z

Although Wow64DisableWow64FsRedirection only affects the current thread (which at least means we don't need to worry about concurrency issues), the more work we do with redirect disabled hightens the chance that Windows or a component in this thread may try to load a dll from system32. And while redirection is disabled, this would be the wrong system32. Thus per the documentation of Wow64DisableWow64FsRedirection, it is strongly recommended to only do this around the lowest API calls possible.
Thus, rather than wrapping _setProductInfo, it may be better as a with-statement around the call to getFileVersionInfo in _getExecutableFileInfo.
I'm assuming that code inside _getImmersivePackageInfo does not require redirection to be disabled?

…rPath`

…n for `_setProductInfo`

lukaszgo1 · 2021-10-21T18:09:25Z

I believe all requested changes are now addressed.

I've removed SHGetFolderPath and replaced all calls with SHGetKnownFolderPath.
Wow64 redirection is disabled only for the duration of fileUtils.getFileveVersionInfo.
To avoid initialization of variables at the module level wrapper around SHGetKnownFolderPath caches results as not to call the win32 function unnecessarily.

seanbudd · 2021-10-22T01:59:03Z

source/shlobj.py

-
-
-class CSIDL(enum.IntEnum):
+class FOLDERID(enum.Enum):


Suggested change

class FOLDERID(enum.Enum):

class FOLDERID(str, enum.Enum):

Would you be able to explain what this change is for? I'm going to apply it regardless as not to block this from further reviews, but this pattern is new to me.

This adds type information to the Enum

Some examples and more information:

https://docs.python.org/3/library/enum.html#restricted-enum-subclassing

https://docs.python.org/3/library/enum.html#others

seanbudd · 2021-10-22T03:02:52Z

@michaelDCurran - thoughts on the updated approach? I've reviewed the code otherwise

michaelDCurran · 2021-10-22T04:01:27Z

What would happen if there was a 32-bit app in c:\windows\syswow64 (c+\windows\system32 from the point of view of the app in question), and NVDA tried to get its file version info?
The file path according to that app would be c+\windows\system32\blah.exe
NVDA would disable wow64 redirection and then pass it to getfileVersionInfo. But then that means it would look in the 64-bit system32 directory. this is wrong.
Is this handled some how?
I would have thought we should only suspend wow64 redirection inside _setProduct info, just around the call to fileUtils.getFileVersionInfo, but only if the process we got the file path from is in fact 64-bit.
Apologies if I've missed code that somehow already considers this.

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

…lukaszgo1/nvda into file-info-for-system32-binaries

lukaszgo1 · 2021-10-22T07:54:20Z

What would happen if there was a 32-bit app in c:\windows\syswow64 (c+\windows\system32 from the point of view of the app in question), and NVDA tried to get its file version info? The file path according to that app would be c+\windows\system32\blah.exe NVDA would disable wow64 redirection and then pass it to getfileVersionInfo. But then that means it would look in the 64-bit system32 directory. this is wrong. Is this handled some how? I would have thought we should only suspend wow64 redirection inside _setProduct info, just around the call to fileUtils.getFileVersionInfo, but only if the process we got the file path from is in fact 64-bit. Apologies if I've missed code that somehow already considers this.

There is no special code for this, but that is because this is handled well by Windows itself. To make sure I'm right I've tested as follows:

Started notepad on 64-bit Windows ensured that its file path as returned from the call to focus.appModule.appPath is 'C:\\Windows\\System32\\notepad.exe'.
Started Notepad from c:\Windows\syswow64 on 64-bit Windows and ensured that the path as returned from the call to focus.appModule.appPath is 'C:\\Windows\\SysWOW64\\notepad.exe.

michaelDCurran · 2021-10-22T22:45:52Z

@lukaszgo1 thanks for confirming that. Just to be doubly sure, I also placed a test 32-bit executable file in c:\windows\syswow64, and then started it from inside NVDA (I.e. from within another 32-bit process) with

subprocess.Popen('c:\\windows\\system32\\blah.exe')

And verified that NVDA's appModule for that executable had an appPath of
c:\windows\syswow64\blah.exe.
So this all looks good to me.

lukaszgo1 · 2021-10-25T06:20:17Z

@seanbudd When adding change log entry for this PR you've forgot the one listed under the 'changes for developers' section.

LeonarddeR · 2021-10-25T10:40:10Z

The following seems to fail:

shlobj.SHGetKnownFolderPath(shlobj.FOLDERID.ProgramData)

Traceback (most recent call last):
  File "<console>", line 1, in <module>
  File "shlobj.pyc", line 44, in SHGetKnownFolderPath
  File "comtypes\GUID.pyc", line 35, in __init__
  File "_ctypes/callproc.c", line 935, in GetResult
OSError: [WinError -2147221005] Invalid class string

Update: AH, seems I have to append value to get the real string. This approach seems a bit confusing. Why not make the enum a comtypes.GUID enum?

Also I'm slightly concerned about the new FOLDERID enum. We generally use Pascal case for class and enum names and upper case names for enum values. See controlTypes.Role and controlTypes.State.

lukaszgo1 · 2021-10-25T11:29:51Z

Update: AH, seems I have to append value to get the real string. This approach seems a bit confusing. Why not make the enum a comtypes.GUID enum?

You cannot create an enum specifying comtypes.GUID as its type - it causes metaclass conflict (not sure why as I gave it only a cursory look).
Even if I would not specify the type of enum member and just assign comtypes.GUID to the member FOLDERID.RoamingAppData would return a member of FOLDERID not a comtypes.GUID so accessing a value is still required.

Having said that I am not fully satisfied with the current solution, so if you have better ideas please do tell.

Also I'm slightly concerned about the new FOLDERID enum. We generally use Pascal case for class and enum names and upper case names for enum values. See controlTypes.Role and controlTypes.State.

My understanding, (although it seems this is not covered by our coding standards) was that we follow the style that you've described above for our own enumerations, but when the given enum merely maps win32 constants we attempt to follow Windows conventions. If there is general consensus that we should follow our own style even in these cases I'm happy to rename as appropriate but this should certainly be documented in the coding standards. For another example where I followed the same logic see #12753 in particular languageHandler.LOCALE.

seanbudd · 2021-10-25T23:23:32Z

@seanbudd When adding change log entry for this PR you've forgot the one listed under the 'changes for developers' section.
We generally use Pascal case for class and enum names and upper case names for enum values. See controlTypes.Role and controlTypes.State.

Thanks for point this out - I will draft a PR (#12986) to fix this and we can discuss the casing issue. CamelCase is the standard we have discussed, but as @lukaszgo1 mentions, this hasn't been consistent.

Why not make the enum a comtypes.GUID enum?

I'm not sure if that will resolve that issue, like @lukaszgo1 mentions, but I think it would be an improvement. I'll draft a PR that creates a base GUIDEnum, like the DisplayString ones, which resolves the metaclass conflict. This shouldn't be a breaking API change, or need to be documented, so this will be a separate PR.

seanbudd · 2021-10-26T01:03:47Z

I'm not sure if that will resolve that issue, like @lukaszgo1 mentions, but I think it would be an improvement. I'll draft a PR that creates a base GUIDEnum, like the DisplayString ones, which resolves the metaclass conflict. This shouldn't be a breaking API change, or need to be documented, so this will be a separate PR.

Actually resolving this seems too difficult to be worth doing. The following (standard) pattern relies on __new__, which is unsupported by comtypes structures https://stackoverflow.com/a/62868487/8030743

class _GUIDEnumMeta(EnumMeta, type(GUID)):
	pass

class GUIDEnum(GUID, Enum, metaclass=_GUIDEnumMeta):
	pass

class FolderId(GUIDEnum):
	example = GUID("{}")
	
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\sean\AppData\Local\Programs\Python\Python37-32\lib\enum.py", line 175, in __new__
    enum_class = super().__new__(metacls, cls, bases, classdict)
TypeError: _ctypes.PyCStructType.__new__(_GUIDEnumMeta) is not safe, use type.__new__()

zstanecic · 2021-10-27T15:33:56Z

Note: this pull request made NVDA remote incompatible. appropriate changes to the NVDA remote should be made.

Fixup of #12943 Summary of the issue: Changelog entries were missing for #12943 UpperCamelCase is the standard for class names, including enums, however FOLDERID was used as the casing for the enum, which mirrors the windows constants. There is a similar issue for the enum members (which used UpperCamelCase instead of CAP_SNAKE_CASE). If we want to change our practice to be consistent with what an Enum represents, we should update codingStandards.md Also the enum type was not being fully leveraged by SHGetKnownFolderPath, requiring .value to be used unnecessarily when calling the function. Description of how this pull request fixes the issue: Updates the casing of the enum and it's members. Updates the change log, and fixes up some earlier entries.

…s to handle paths from different disks (#13009) PR #12943 improved code which retrieves version information for a given executable files placed in system32 under 64-bit versions of Windows. To do so it is necessary to check if path of the given binary starts with path of system32 directory for the current system. The recommended way to compare path's in a case insensitive way was os.pathcommonPath`, however it turns out it does not work when path's are from different drives. In case of reporter of #13008 they had Office installed in a custom location probably on non system disk. Description of how this pull request fixes the issue: This PR simplifies the code to use standard str.casefold for comparisons.

lukaszgo1 added 4 commits October 16, 2021 16:30

Introduce context manager for pausing Wow64 for a section of code in …

7d3f89c

…winKernel

Lint for shlobj"

2ef4a97

- Clean start imports - Update copyright header - Use enums and add some new CSIDL constants

Remove unused getSystemConfigPath

85ab8ae

Ensure that NVDA can retrieve file version info for binaries placed i…

10c837b

…n systes32 by temporarily suspending SysWOW64 redirection

LeonarddeR suggested changes Oct 18, 2021

View reviewed changes

lukaszgo1 added 2 commits October 18, 2021 19:01

Merge branch 'master' into file-info-for-system32-binaries

a25a991

Review actions

7c10407

LeonarddeR added this to the 2022.1 milestone Oct 18, 2021

lukaszgo1 marked this pull request as ready for review October 19, 2021 08:59

lukaszgo1 requested a review from a team as a code owner October 19, 2021 08:59

lukaszgo1 requested a review from seanbudd October 19, 2021 08:59

seanbudd reviewed Oct 19, 2021

View reviewed changes

lukaszgo1 added 3 commits October 21, 2021 15:06

Merge branch 'master' into file-info-for-system32-binaries

fed9ac0

Remove deprecated SHGetFolderPath and replace with `SHGetKnownFolde…

d94e309

…rPath`

Disable syswow64 redirection just for getFileVersionInfo rather tha…

1816b34

…n for `_setProductInfo`

seanbudd approved these changes Oct 22, 2021

View reviewed changes

seanbudd requested a review from michaelDCurran October 22, 2021 03:02

lukaszgo1 and others added 3 commits October 22, 2021 09:42

Merge branch 'master' into file-info-for-system32-binaries

fdba9dc

Add additional class to the enum

e2896bc

Co-authored-by: Sean Budd <seanbudd123@gmail.com>

Merge branch 'file-info-for-system32-binaries' of https://github.com/…

0600845

…lukaszgo1/nvda into file-info-for-system32-binaries

michaelDCurran approved these changes Oct 22, 2021

View reviewed changes

update changes

177ab51

seanbudd merged commit 3ce266b into nvaccess:master Oct 25, 2021

lukaszgo1 deleted the file-info-for-system32-binaries branch October 25, 2021 06:05

seanbudd mentioned this pull request Oct 25, 2021

Fixup FolderId usage and change log #12986

Merged

5 tasks

lukaszgo1 mentioned this pull request Oct 30, 2021

Fix-up of PR 12943 - Avoid os.path.commonPath` when comparing paths of the binaries as it does not work for files on different drives #13009

Merged

5 tasks

jcsteh mentioned this pull request Nov 3, 2021

Use shlobj.SHGetKnownFolderPath instead of (now removed) shlobj.SHGetFolderPath. NVDARemote/NVDARemote#283

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that NVDA can get file version information for binaries in system32 on 64-bit versions of Windows #12943

Ensure that NVDA can get file version information for binaries in system32 on 64-bit versions of Windows #12943

lukaszgo1 commented Oct 16, 2021 •

edited

Loading

lukaszgo1 commented Oct 16, 2021

LeonarddeR Oct 18, 2021

lukaszgo1 Oct 18, 2021

LeonarddeR Oct 18, 2021

seanbudd Oct 19, 2021

LeonarddeR Oct 18, 2021

lukaszgo1 Oct 18, 2021

LeonarddeR Oct 18, 2021

lukaszgo1 commented Oct 19, 2021

seanbudd left a comment

seanbudd Oct 19, 2021

michaelDCurran commented Oct 20, 2021

lukaszgo1 commented Oct 21, 2021

seanbudd Oct 22, 2021

lukaszgo1 Oct 22, 2021

seanbudd Oct 25, 2021

seanbudd commented Oct 22, 2021

michaelDCurran commented Oct 22, 2021

lukaszgo1 commented Oct 22, 2021

michaelDCurran commented Oct 22, 2021

lukaszgo1 commented Oct 25, 2021

LeonarddeR commented Oct 25, 2021 •

edited

Loading

lukaszgo1 commented Oct 25, 2021

seanbudd commented Oct 25, 2021 •

edited

Loading

seanbudd commented Oct 26, 2021

zstanecic commented Oct 27, 2021

Ensure that NVDA can get file version information for binaries in system32 on 64-bit versions of Windows #12943

Ensure that NVDA can get file version information for binaries in system32 on 64-bit versions of Windows #12943

Conversation

lukaszgo1 commented Oct 16, 2021 • edited Loading

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing strategy:

Known issues with pull request:

Change log entries:

Code Review Checklist:

lukaszgo1 commented Oct 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukaszgo1 commented Oct 19, 2021

seanbudd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelDCurran commented Oct 20, 2021

lukaszgo1 commented Oct 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seanbudd commented Oct 22, 2021

michaelDCurran commented Oct 22, 2021

lukaszgo1 commented Oct 22, 2021

michaelDCurran commented Oct 22, 2021

lukaszgo1 commented Oct 25, 2021

LeonarddeR commented Oct 25, 2021 • edited Loading

lukaszgo1 commented Oct 25, 2021

seanbudd commented Oct 25, 2021 • edited Loading

seanbudd commented Oct 26, 2021

zstanecic commented Oct 27, 2021

lukaszgo1 commented Oct 16, 2021 •

edited

Loading

LeonarddeR commented Oct 25, 2021 •

edited

Loading

seanbudd commented Oct 25, 2021 •

edited

Loading