About Dialog OS Version

[mkitzan]

Summary of the Pull Request

Adds the user's full Windows version to the About dialog page. This reduces the amount of work a user must perform to open an Issue.

References

https://docs.microsoft.com/en-us/windows/win32/sysinfo/getting-the-system-version

PR Checklist

• Closes Provide Windows version in the about box #3616
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Requires documentation to be updated
• I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: Provide Windows version in the about box #3616

Detailed Description of the Pull Request / Additional comments

PR adds a function _GetWindowVersion to TerminalPage which queries the version of an important Sytem32 DLL. The About dialog page uses this function to query the system version. While the referenced link on getting the Windows version suggests querying kernel32.dll, I found its version number was inconsistent compared to the version given when ver is run in a cmd terminal. This implementation queries KernelBase.dll instead which gives the correct version.

PR also changes the Windows Terminal version line to read WT Version: x.x.x.x to clearly differentiate the OS and WT versions.

Validation Steps Performed

To validate the correctness of the About dialog OS version:

• Run the command ver in a cmd terminal
• Open the About dialog page in WT
• Compare the two Windows version numbers

To test error checking and case where DLL does not exist, I replaced KernelBase.dll with does_not_exist.dll and re-ran WT and got the expected OS Version: 0.0.0.0 <arch>.

[Biswa96]

Some thoughts:

• WT may be "Windows Terminal".
• RtlGetVersion can be used which gets version from PEB.
• Add the architecture for both Windows and Windows Terminal.
• A small logo and/or copyright and/or GitHub repo link can be added.

[mkitzan]

@Biswa96 RtlGetVersion would require introducing a new external dependency which seems a little overkill for this feature. The three hyper links in the About body link to the WT repo. As for the architecture, if you know a good way of querying the system architecture, I'd be up for adding it.

[Biswa96]

RtlGetVersion is from ntdll which may not be any overhead. For architecture, GetSystemInfo or GetNativeSystemInfo may be used.

[mkitzan]

Unless I'm mistaken to use RtlGetVersion, header file(s) wdm.h or Ntddk.h are required. Both of which are not present in the External Dependencies of TerminalAppLib. I'm looking into adding the architecture, thanks for the info.

[zadjii-msft]

Thanks for the contribution! I think this is fine, but I'm going to hold off signing off until the headers get removed.

[zadjii-msft]

I'm pretty sure these should actually already both be included in LibraryIncludes in pch.h, so they shouldn't be needed here.

[mkitzan]

Yeah pch.h was including them. Committed the fix to this.

[zadjii-msft]

I think I'd personally leave this unchanged, but maybe that's a personal stylistic choice. @DHowett-MSFT /@cinnamon-msft do you have thoughts here?

(TBH you don't need to change this back unless someone else agrees with me here)

[metathinker]

If you can, you might want to try your changed Terminal in Windows 10 version 1909 (the November 2019 update).

I'm worried it will not correctly report the Windows build number of 18363 for that version - because of the way 1909 is delivered (it is 1903 plus a small "enablement package" update to change a feature flag value), all Windows file versions in 1909 match those of 1903. Hence, if an application checks kernelbase.dll's version like you do here, it'll get 1903's build number of 18362, not 1909's build number of 18363.

[metathinker]

Also, while @Biswa96 is correct that RtlGetVersion() is exported from NtDLL.dll and thus can be used from user-mode programs, that fact is not documented. Which could put you in a bit of trouble, unfortunately - there are good reasons why RtlGetVersion() is not documented as usable from user-mode code, but being unable to use it breaks your use case.

[mkitzan]

@metathinker updated to Windows version 1909, and as you expected the update was not reflected in the DLL. Looking at the DLL modules WT is utilizing, none of them reflect this new version change either. Definitely seems questionable to use undocumented functionality, but I could implement it if needed.

[Biswa96]

RtlGetVersion() isn't undocumented. One can mention the function signature atop of the calling function. And linker will do the rest. Also it doesn't depend on any file, the members of output struct comes directly from PEB.

[metathinker]

I apologize for sounding nitpicky, but when programs start doing things that Microsoft doesn't support for use outside of Windows itself, they rapidly create trouble for themselves and for Microsoft's Windows application compatibility team.

RtlGetVersion() isn't undocumented.

Only the kernel-mode version of RtlGetVersion in ntoskrnl.exe is documented. The user-mode version in ntdll.dll is undocumented, as is almost every function exported by ntdll.dll.

One can mention the function signature atop of the calling function. And linker will do the rest.

Yes, you can do that, but that doesn't mean you should. RtlGetVersion is probably okay, but if you start calling things like NtDirectGraphicsCall (to pick a random export from ntdll) then you are going to be in real trouble in a few years, or less if you're lucky.

Also it doesn't depend on any file, the members of output struct comes directly from PEB.

It's true that the user-mode version of RtlGetVersion merely reads the process environment block. But there is a good reason we (Microsoft) don't document the version number fields in the PEB so you could just read from it yourself: We don't want people checking the version of Windows.

Inevitably people do version checks wrong, so often that we added a feature to Windows such that documented version-check APIs lie to you by default. As it happens, calling RtlGetVersion is currently a way to bypass that check. But it is undocumented precisely to tell you "maybe you should try another path".

[mkitzan]

@metathinker and @Biswa96 thanks for all the feedback and info. I'll start looking into implementing this with RtlGetVersion.

[DHowett-MSFT]

@mkitzan sorry -- i think the message was corrupted in transit. I don't think we should move forward with RtlGetVersion just yet 😄

[mkitzan]

@DHowett-MSFT probably was haha. I'll hold fast then, thanks.

[wjk]

A suggestion. Is there any reason why we cannot or should not read the CurrentBuild value under HKEY_LOCAL_MACHINE\SOFTWARE\Microsoft\Windows NT\CurrentVersion? AFAIK, this Registry value always reflects the current Windows build number. (Edited for clarity.)

[WSLUser]

I have a nitpick that hopefully might be accepted from a visual perspective. Could we make "WT Version" and "OS Version" display in bold?

[miniksa]

I am asking the expert on Windows OS versioning what the most appropriate way is to retrieve this data. I know that checking kernelbase.dll is not it. But I forget which way is the right way. When he answers, we'll do that one.

[miniksa]

FYI, I am pretty sure his answer is going to be "don't check the OS version for anything."

I'm also not certain that we have anything in here that is strictly dependent on the OS version anymore.

[WSLUser]

FYI, I am pretty sure his answer is going to be "don't check the OS version for anything."

I'm also not certain that we have anything in here that is strictly dependent on the OS version anymore.

We still require the OS Version for submitting issues, which is what this PR is addressing. A problem brought up for Terminal may in fact be for ConPTY or conhost but with the About Dialog, we can quickly copy/paste the OS Version so in future versions of Windows, we can still quickly determine if the issue brought up was resolved already or even a potential regression. Also, we need to ensure that Windows 10x is supported as well. (probably will be but just a sanity check)

[mkitzan]

This is a brain dead hack, but we probably could open a ConPTY connection, have it execute ver, and just spit back the results to About.

[miniksa]

I have heard the word of our savior, the OS versioning master himself.

1. Don't do it because you probably don't need it.
2. If you want to present a build number and you have to, apply the manifest declaring that you know about Windows 10 (see SupportedOS on this page: https://docs.microsoft.com/en-us/windows/win32/sbscs/application-manifests) and call GetVersionEx and show the number out of that.

[mkitzan]

Bless the OS versioning master for condescending themselves to this most humble of issues 🙏
I'll look into getting option 2 working, unless the big brain move is option 1.

[metathinker]

Well... calling GetVersion() instead of RtlGetVersion() will subject you to the version lie by default that I mentioned above, and the manifest opt out only works for a particular major version of Windows, but as long as Windows' main version is stuck at 10, I doubt that will matter :)

I believe you'd want to modify this file: https://github.com/microsoft/terminal/blob/master/src/cascadia/WindowsTerminal/WindowsTerminal.manifest
to add the GUID for Windows 10 as shown here:
https://docs.microsoft.com/en-us/windows/win32/w8cookbook/windows-version-check

Or at least you would so modify that file, but it already appears to have the right <supportedOS> element for Windows 10, so it may not need to be changed.

[miniksa]

as long as Windows' main version is stuck at 10, I doubt that will matter :)

Correct. That's why we're going to prefer using the more supported-from-user-mode-code GetVersionEx over jumping straight to the DDK's Rtl version.

[mkitzan]

Thanks for the supportedOS GUID check/info @metathinker, good to know the manifest already should be set for Windows 10. I'll try getting GetVersionEx working, but I swear I tried it much earlier.

[mkitzan]

Gave it shot, and now I remember why the function was familiar... GetVersionEx is deprecated so the build won't compile.

[miniksa]

Gave it shot, and now I remember why the function was familiar... GetVersionEx is deprecated so the build won't compile.

Nnnnnnn.

#pragma warning(suppress:4996) // Better the deprecated official API than the Rtl or some other more complex and less reliable method of getting the OS build number so people can put it in the bug template. Also, I'm pretty sure an oil tanker will explode in the gulf of somewhere if we ever actually remove this API from Windows.

Or we should just give it up and take the version number out of the template or something.

@DHowett-MSFT, opinion?

[DHowett-MSFT]

I'm okay with the suppression. The value in having the OS version is reduced every time we take a component out-of-box, though, so I dunno if we even need this. It is nice to have a one-stop shop for version numbers though.

[mkitzan]

Suppressing a warning and using a deprecated function for a garnish feature like this seems a little off. I can close this PR, and just keep the branch live on my fork. If we ever get a fully supported option, then it'll be easy to plug it into branch and re-open the PR.

Let me know your thoughts, @miniksa / @DHowett-MSFT, before I go for it.

[DHowett-MSFT]

@mkitzan you’ve got good instincts :)

[DHowett-MSFT]

(Also, thank you!)

[mkitzan]

Thanks for all the reviewing and advice on this one! I wouldn't have guessed OS versioning would be such a tricky subject.