CsWin32: `BOOL`/`BOOLEAN` templates are incorrect

The BOOL and BOOLEAN template constructors taking a C# bool are invalid.

https://github.com/microsoft/CsWin32/blob/ab63c9ad139a77a43ad331f7334d38799d6125e2/src/Microsoft.Windows.CsWin32/templates/BOOL.cs#L3

https://github.com/microsoft/CsWin32/blob/ab63c9ad139a77a43ad331f7334d38799d6125e2/src/Microsoft.Windows.CsWin32/templates/BOOLEAN.cs#L3

The BOOL template should be, the BOOLEAN is similar.

internal unsafe BOOL(bool value) => this.Value = value ? 1 : 0;

The notion of simply blitting the lowest bits of a bool instance is undefined. It also makes this usage troublesome if a user defines SkipLocalsInitAttribute, which with the current implementation leave the upper bits of Value in an undefined state thus making a .NET false possibly be non-zero.

About this issue

Original URL
State: closed
Created 2 years ago
Reactions: 1
Comments: 26 (26 by maintainers)

Commits related to this issue

Fixes #624 — committed to elachlan/CsWin32 by elachlan 2 years ago
changes from discussions in #624 — committed to elachlan/CsWin32 by elachlan 2 years ago
Avoid overflow exceptions when converting `BOOL` to `bool` This means it has to be a lossy conversion, since `BOOL` is 4 bytes and `bool` is 1 byte. Generally .NET recommends avoiding lossy implicity... — committed to microsoft/CsWin32 by AArnott 2 years ago
Avoid overflow exceptions when converting `BOOL` to `bool` This means it has to be a lossy conversion, since `BOOL` is 4 bytes and `bool` is 1 byte. Generally .NET recommends avoiding lossy implicity... — committed to microsoft/CsWin32 by AArnott 2 years ago
Avoid overflow exceptions when converting `BOOL` to `bool` This means it has to be a lossy conversion, since `BOOL` is 4 bytes and `bool` is 1 byte. Generally .NET recommends avoiding lossy implicity... — committed to microsoft/CsWin32 by AArnott 2 years ago

Most upvoted comments

I think its because some Win32APIs return BOOL, but indicate -1 for an error state. https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getmessage https://github.com/microsoft/CsWin32/issues/339 https://github.com/microsoft/CsWin32/issues/362 https://github.com/microsoft/CsWin32/pull/363

Thank git and its version history for me being able to track that down.

elachlan on Jul 28, 2022

@AaronRobinsonMSFT , do we just go with this?

Sure or, even simpler - return this.value != 0;

AaronRobinsonMSFT on Jul 28, 2022

It is basically assuming the runtime will define its bool ABI to be what the interop layer decided is appropriate.

The runtime cannot change the memory layout of bool at this point, can it? We expect it to be 1 byte long, and we may copy 1 byte into that memory. C# emits IL for bool conditions that treat any non-zero value as truthy, just as C does. I’m struggling to see how it’s problematic then to copy the value from BOOL or BOOLEAN into bool in an implicit conversion so that another implicit conversion can change it back. And this isn’t me just trying to be clever… I had to work this out to resolve another issue (linked above).

AArnott on Jul 30, 2022

CC: @RussKie

elachlan on Jul 28, 2022

making a .NET false possibly be non-zero.

Wow, that’s a good caveat to know. Is that just with the default value, or even if you then assign false?

With the current implementation, even if a .NET false value was set, it would only set the least significant byte in the Value field. This means that if the higher 3 bytes are non-zero due to not being explicitly initialized to zero then only the lowest byte would be set to the assumed value of 0.

AaronRobinsonMSFT on Jul 27, 2022