lhecker / windows-dark-mode-switcher

2 stars 1 forks source link

`SendNotifyMessage()` might not be the right function to broadcast a string #1

Closed german-one closed 14 hours ago

german-one commented 20 hours ago

When I've seen you using this function I remembered that I tried it out for exactly the same reason some time ago. The difference is that I wrapped it into a piece of C# code to make it usable in a PowerShell Add-Type definition. At least the function name indicates that it is made for this purpose and I found it quite comfortable that it returns instantly and that it is still readable with only four parameters, compared with the seven parameters of SendMessageTimeout().
However, SendNotifyMessage() didn't work reliably for me. Every now and then the mode didn't change immediatelly. E.g. the color of the taskbar didn't change or taskbar icons turned into something like high contrast style. I can't reproduce this bug with your C code. Using SendMessageTimeout() was what I had to do to fix my code though. SendNotifyMessage() returns immediatelly when used along whith HWND_BROADCAST, which makes the string pointer it sends to the message queues a dangling pointer.

https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-sendnotifymessagew#remarks

If you send a message in the range below WM_USER to the asynchronous message functions (PostMessage, SendNotifyMessage, and SendMessageCallback), its message parameters cannot include pointers. Otherwise, the operation will fail. The functions will return before the receiving thread has had a chance to process the message and the sender will free the memory before it is used.

lhecker commented 16 hours ago

[...] and the sender will free the memory before it is used.

That's an unusually bold assumption by the author of the documentation. Not all pointers must refer to dynamic memory allocations. In this case it refers to a string literal, which resides in the constant .rodata segment of the application. In other words, it'll never become invalid, because it's a pointer into global constant storage.

But this raises another great point that I never thought about: When you send out a HWND_BROADCAST message, how do other applications access your pointer? Since the string literal resides in my application's address space, other applications cannot possibly access that memory just by using the pointer as is. This proves that the kernel must be copying the LPARAM string into each receiver's address space.

I guess that means that using SendNotifyMessage with such a literal is safe?

german-one commented 15 hours ago

But this raises another great point that I never thought about: When you send out a HWND_BROADCAST message, how do other applications access your pointer? Since the string literal resides in my application's address space, other applications cannot possibly access that memory just by using the pointer as is. This proves that the kernel must be copying the LPARAM string into each receiver's address space. This proves that the kernel must be copying the LPARAM string into each receiver's address space.

My assumption was indeed that the string is copied. Now it depends what it actually means if SendNotifyMessage returns "immediatelly". If sending the message to all top level windows happens if the function already returned, then you're already about to tear down your process as the next thing in your code is returning from main. So, the address space of your process is getting invalid, and .rodata or not doesn't matter anymore. At least that's my humble understanding πŸ˜…

lhecker commented 15 hours ago

Yeah, so I think SendNotifyMessage should be ideal for this scenario. It just posts the notification into the message queue of all processes and then immediately exits without waiting (= efficient). Since the OS copies the string literal there are no lifetime/reference issues.

Apropos...

[...] as the next thing in your code is returning from main.

I'm currently working on the first "full" version of this tool over in the https://github.com/lhecker/windows-dark-mode-switcher/tree/wip branch. My goal is to make the scheduled time configurable, as well as allowing the tool to register itself as a scheduled task. So basically, you just put it somewhere run dark-mode-switcher.exe register 06:00 18:00 and it'll do the entire event scheduler thing for you. I just haven't had enough time last weekend to finish it. πŸ˜… (If you want to give it a try to finish the branch, please feel free to do so. Otherwise, I'll let you know once I'm done, if you're interested in this project.)

german-one commented 14 hours ago

I'm still wondering what difference it makes if it is p/invoked from a PowerShell script then. However, that's not a topic that belongs to here 😊 I'll definitely try your update and let you know. (COM interfaces in C are fun πŸ˜„)
BTW you could optionally let the users pass their longitude and latitude and calculate the real sunrise and sunset time from the current date. Of course respecting daylight saving time and stuff 🀣 Although I'm afraid you'll never be bored enough to implement things like that.

lhecker commented 14 hours ago

I'm still wondering what difference it makes if it is p/invoked from a PowerShell script then.

Was your issue with string literals or another type of pointer? And was it with a HWND_BROADCAST message? I wonder if it fails to work in C# because it copies string literals into the heap? Perhaps it GC's them at random times...

Although I'm afraid you'll never be bored enough to implement things like that.

That's actually right up my alley. πŸ˜…

german-one commented 14 hours ago

I don't remember the old code in detail. It's been a predecessor of that code with SendNotifyMessage in place of SendMessageTimeout.

Click to expand ```Powershell Add-Type @' using Microsoft.Win32; using System; using System.Runtime.InteropServices; namespace Theme { public enum Mode { Dark, Light, Toggle } public static class P13n { private static readonly IntPtr n0 = IntPtr.Zero, NULL = IntPtr.Zero, HWND_BROADCAST = new IntPtr(0xFFFF); private const int WM_SETTINGCHANGE = 0x001A, SMTO_ABORTIFHUNG = 0x0002; private const string areaICS = "ImmersiveColorSet"; [DllImport("kernel32.dll", SetLastError = true)] private static extern void SetLastError(int err); [DllImport("user32.dll", CharSet = CharSet.Unicode, SetLastError = true)] private static extern IntPtr SendMessageTimeout(IntPtr hw, int m, IntPtr wp, string lp, int f, int tout, IntPtr r); private static void BroadcastP13nChanges() { SetLastError(0); if (n0 == SendMessageTimeout(HWND_BROADCAST, WM_SETTINGCHANGE, n0, areaICS, SMTO_ABORTIFHUNG, 5000, NULL) || 0 != Marshal.GetLastWin32Error()) { throw new TimeoutException("Setting changes broadcasted incompletely."); } } private static RegistryKey GetP13nKey() { return Registry.CurrentUser.CreateSubKey(@"Software\Microsoft\Windows\CurrentVersion\Themes\Personalize"); } private static bool SetBoolAsI4(string name, bool one) { bool old; using(RegistryKey p13n = GetP13nKey()) { old = (0 != (int)p13n.GetValue(name, 0)); p13n.SetValue(name, one ? 1 : 0); } BroadcastP13nChanges(); return old; } public static Mode SetMode(Mode mode) { Mode old; using(RegistryKey p13n = GetP13nKey()) { old = (0 == (int)p13n.GetValue("SystemUsesLightTheme", 0)) ? Mode.Dark : Mode.Light; int use = (Mode.Light == mode || (Mode.Toggle == mode && Mode.Dark == old)) ? 1 : 0; p13n.SetValue("SystemUsesLightTheme", use); p13n.SetValue("AppsUseLightTheme", use); } BroadcastP13nChanges(); return old; } public static bool EnableTransparency(bool apply) { return SetBoolAsI4("EnableTransparency", apply); } public static bool EnableTaskbarColor(bool apply) { return SetBoolAsI4("ColorPrevalence", apply); } } } '@ # Start > Settings > Personalization > Colors > ... # ... Choose your mode $prevMode = [Theme.P13n]::SetMode([Theme.Mode]::Toggle) Write-Output "Previous mode: $prevMode" # ... Transparency effects #$wasTransp = [Theme.P13n]::EnableTransparency($True) #Write-Output "Transparency effects were used: $wasTransp" # ... Show accent color on Start and taskbar #$hadTaskbarCol = [Theme.P13n]::EnableTaskbarColor($False) #Write-Output "Start and taskbar were colored: $hadTaskbarCol" pause ```


That's actually right up my alley. πŸ˜…

That'd be so cool 🀣