Skip to content
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

OSVERSIONINFOEX uses incorrect charset with RtlGetVersion() #2095

Closed
kpreisser opened this issue May 14, 2018 · 1 comment
Closed

OSVERSIONINFOEX uses incorrect charset with RtlGetVersion() #2095

kpreisser opened this issue May 14, 2018 · 1 comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior in progress 🚧 win 3️⃣2️⃣
Milestone

Comments

@kpreisser
Copy link

kpreisser commented May 14, 2018

I'm submitting a...

  • Bug report (I searched for similar issues and did not find one)

Current behavior

Hi, please see the following code in OSVersionHelper.cs (branch rel/3.0.0-preview):
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/280ae1ed883e1c46bbc491c4545f52808e818fa2/Microsoft.Toolkit.Win32.UI.Controls/Interop/OSVersionHelper.cs#L128-L129

Note that the comment indicates that RtlGetVersion() doesn't set the ProductType in the OSVERSIONINFOEX structure. However, actually RtlGetVersion() sets this property, but OSVERSIONINFOEX isn't set to use Unicode, which is why the properties are not correctly filled as the string field (CSDVersion) will have the incorrect size (128 instead of 256 bytes).

Currently, the struct is defined as follows:
https://github.com/windows-toolkit/WindowsCommunityToolkit/blob/280ae1ed883e1c46bbc491c4545f52808e818fa2/Microsoft.Toolkit.Win32.UI.Controls/Interop/Win32/OSVERSIONINFOEX.cs#L17-L18

Here, Marshal.SizeOf(typeof(OSVERSIONINFOEX)) will return 156.

Expected behavior

The declaration should be changed to:

    [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)]
    internal struct OSVERSIONINFOEX

In that case, Marshal.SizeOf(typeof(OSVERSIONINFOEX)) will return 284, and the remaining properties like ProductType should now be set correctly.

Note that the declaration of GetVersionEx() should then also be changed from

    [DllImport(ExternDll.Kernel32, SetLastError = true, EntryPoint = "GetVersionEx")]
    private static extern bool _GetVersionEx(ref OSVERSIONINFOEX osVersionInfo);

to e.g.:

    [DllImport(ExternDll.Kernel32, SetLastError = true, EntryPoint = "GetVersionExW", CharSet = CharSet.Unicode)]

so that it uses the wide variant (GetVersionExW). But I think it can also be removed because RtlGetVersion should work correctly now.

Environment

Nuget Package(s): Microsoft.Toolkit.Win32.UI.Controls

Package Version(s): 3.0.0-preview

Windows 10 Build Number:
- [X] April 2018 Update (17134)

App min and target version:
- [X] April 2018 Update (17134)

Device form factor:
- [X] Desktop

Visual Studio 
- [X] 2017 (version: 15.7.0)

Thank you!

@kpreisser kpreisser changed the title OSVERSIONINFOEX uses incorrect charset with RtlGetVersion OSVERSIONINFOEX uses incorrect charset with RtlGetVersion() May 14, 2018
@nmetulev nmetulev added bug 🐛 An unexpected issue that highlights incorrect behavior win 3️⃣2️⃣ labels May 14, 2018
@rjmurillo rjmurillo added this to the 3.0 milestone May 15, 2018
rjmurillo added a commit to rjmurillo/UWPCommunityToolkit that referenced this issue May 15, 2018
rjmurillo added a commit to rjmurillo/UWPCommunityToolkit that referenced this issue May 15, 2018
With fix 0ca3224 and CommunityToolkit#2095 resolved, `GetVersionEx` is no longer required.
rjmurillo added a commit to rjmurillo/UWPCommunityToolkit that referenced this issue May 15, 2018
With fix 0ca3224 and CommunityToolkit#2095 resolved, `GetVersionEx` is no longer required.
@rjmurillo rjmurillo mentioned this issue May 15, 2018
7 tasks
@nmetulev
Copy link
Contributor

PR merged

@ghost ghost locked as resolved and limited conversation to collaborators Nov 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior in progress 🚧 win 3️⃣2️⃣
Projects
None yet
Development

No branches or pull requests

3 participants