MAKEINTRESOURCE integers should be 16-bit not 32-bit

[meowtec]

Which crate is this about?

windows

Crate version

0.39

Summary

TD_WARNING_ICON was missing in older version and introduced in v0.39 (REF: #968), but it seems that the value (PCWSTR(-1i32 as _)) is not correct and can cause STATUS_ACCESS_VIOLATION error when running.

Running `target\debug\message-custom-buttons.exe`
error: process didn't exit successfully: `target\debug\message-custom-buttons.exe` (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

Instead, use PCWSTR(-1i32 as u16 as _) would work correctly.

Toolchain version/configuration

Default host: x86_64-pc-windows-msvc
rustup home: C:\Users\berto.rustup

installed targets for active toolchain

x86_64-pc-windows-gnu
x86_64-pc-windows-msvc

active toolchain

stable-x86_64-pc-windows-msvc (default)
rustc 1.61.0 (fe5b13d68 2022-05-18)

Reproducible example

1. git clone git@github.com:PolyMeilex/rfd.git
2. edit https://github.com/PolyMeilex/rfd/blob/master/src/backend/win_cid/message_dialog.rs#L116 and replace PCWSTR(main_icon_ptr as *const u16) with windows::Win32::UI::Controls::TD_WARNING_ICON
3. cd examples/message-custom-buttons
4. cargo run

Crate manifest

No response

Expected behavior

Should show a task dialog

Actual behavior

error: process didn't exit successfully: target\debug\message-custom-buttons.exe (exit code: 0xc0000005, STATUS_ACCESS_VIOLATION)

Additional comments

No response

[riverar]

This is a bug on win32metadata / me. We should be treating MAKEINTRESOURCE integers as WORDs (16-bit). Will transfer.

[riverar]

#define IS_INTRESOURCE(_r) ((((ULONG_PTR)(_r)) >> 16) == 0)
#define MAKEINTRESOURCEA(i) ((LPSTR)((ULONG_PTR)((WORD)(i))))
#define MAKEINTRESOURCEW(i) ((LPWSTR)((ULONG_PTR)((WORD)(i))))

[sotteson1]

I don't agree that MAKEINTRESOURCE values should be 16-bit. They're passed to functions that are expecting a LPCWSTR or LPSTR. But I don't know enough about the rust projection to know why the repro steps failed to compile. It seems like they should have worked.

[riverar]

@sotteson1 The MAKEINTRESOURCE macros (see above) cast to WORD before doing additional arithmetic. IS_INTRESOURCE also shifts right 16 bits. Am I missing something?

[sotteson1]

The final cast is to a string. They are meant to be used in functions that take a string.

[sotteson1]

IS_INTRESOURCE takes a string and sees if the high word is 0. If it is, you know it’s a 16-bit int resource that has been cast to a string to be used in the resource functions.

[kennykerr]

I'm not sure this is a metadata bug - the metadata represents this as follows:

public const PWSTR TD_WARNING_ICON = -1;

This is completely invalid but also a fairly accurate representation of what the Windows SDK declares. It may be up to the languages to coerce this properly.

[riverar]

When I said we should treat "MAKEINTRESOURCE integers as WORDs" in the original issue, I was referring to the integer input into the macro. Right now, when we scrape MAKEINTRESOURCE(xxx), we treat the xxx as a 32-bit integer before it is natively labeled a PCSTR/PCWSTR. I'm not suggesting we change the string type.

[kennykerr]

Makes sense.

[sotteson1]

Oh, that makes sense. We should fix that.

[riverar]

For curious minds, that STATUS_ACCESS_VIOLATION is a result of the 32-bit integer constant filling the hi-order word. (It also gets sign-extended to fit in a pointer-sized variable/register but the damage is already done at this point.) That 0xffffffffffffffff value is then ferried up the stack, eventually entering user32!FindExistingCursorIcon where a squirrelly "pointer check" is performed (val & FFFFFFFFFFFF0000 != 0). Our value passes this check, so the function happily tries to RtlInitUnicodeString thinking the pointer is valid 💥

[kennykerr]

Thanks for the analysis @riverar - in the meantime I have a workaround here: microsoft/windows-rs#2007

[mikebattista]

Is the fix here just to cast to ushort below?

win32metadata/sources/MetadataUtils/ConstantsScraper.cs

Line 720 in efe67e9

valueText = match.Groups[12].Value;

Or does the fix need to be plumbed through in more places?

[mikebattista]

@riverar any pointers here on the fix?

[riverar]

@mikebattista That sounds right, let's see if we can get a test wired up to verify.

We'll need to fix up that call to AddInteger downstream too. Should have this wrapped up soon.

[riverar]

So I've made some changes that now emits the constant as int16, example IL:

.field public static literal valuetype [Windows.Win32.winmd]Windows.Win32.Foundation.PWSTR TD_WARNING_ICON = int16(-1)

[kennykerr]

Reopening as these constants need to be unsigned to avoid sign extension.

[riverar]

Yep, my dumb mistake. (I even mentioned sign extension smacks head.) Will fix now.