What's wrong with this unmanaged code in C#?

Question

I was trying to get text from each control in hierarchy. The following code runs fine if I use the unsafe method. However, using the unmanaged version seems to break hWnd, which in result hWnd = GetAncestor(hWnd, GetAncestorFlags.GA_PARENT) complains:

System.AccessViolationException: 'Attempted to read or write protected memory. This is often an indication that other memory is corrupt.'

I have checked hWnd was not changed after returning from GetWindowTextRaw function, and if I comment out the second SendMessage in this function will not cause the issue (although it will clearly not get the window text).

(PS: I'm using PInvoke.User32 in NuGet)

// using static PInvoke.User32;

public static string GetWindowTextRaw(IntPtr hWnd) {
    // Allocate correct string length first
    int length = (int)SendMessage(hWnd, WindowMessage.WM_GETTEXTLENGTH, IntPtr.Zero, IntPtr.Zero);
    char[] buff = new char[length + 1];
    IntPtr iptr = Marshal.AllocHGlobal(buff.Length);
    SendMessage(hWnd, WindowMessage.WM_GETTEXT, (IntPtr)(length + 1), iptr);
    Marshal.Copy(iptr, buff, 0, length + 1);
    Marshal.FreeHGlobal(iptr);
    //unsafe
    //{
    //    fixed (char* p = buff)
    //        SendMessage(hWnd, WindowMessage.WM_GETTEXT, (IntPtr)(length + 1), (IntPtr)p);
    //}
    return new string(buff).TrimEnd('\0');
}

private void button1_Click(object sender, EventArgs {
    POINT p;
    IntPtr hWnd;
    //while (true)
    if (GetCursorPos(out p)) {
        hWnd = WindowFromPoint(p); ;
        Debug.Print($"{p.x} {p.y} 0x{(int)hWnd:x8}");
        while (hWnd != IntPtr.Zero) {
            Debug.Print($"{GetWindowTextRaw(hWnd)}");
            hWnd = GetAncestor(hWnd, GetAncestorFlags.GA_PARENT); 
        }
        Thread.Sleep(500);
    }
}

Possible duplicate of [Attempted to read or write protected memory. This is often an indication that other memory is corrupt](http://stackoverflow.com/questions/4074585/attempted-to-read-or-write-protected-memory-this-is-often-an-indication-that-ot) — ThePerplexedOne, May 03 '17 at 12:10
@ThePerplexedOne Thanks for the link, but I don't see anything in common. Would you please give me a better hint? Interestingly, I tried the Release build, and it does not have this issue. Do you have any idea why? Thank! — Tide Gu, May 03 '17 at 12:18
Your marshaling looks overly complicated and error-prone. [Some inspiration](http://stackoverflow.com/a/7740920/4137916). Depending on your use case, you may also want to consider using [UI Automation](https://msdn.microsoft.com/library/ms747327) rather than low-level `WM_GETTEXT` groveling. — Jeroen Mostert, May 03 '17 at 12:21
@JeroenMostert Yes, if I overload SendMessage, I can pass StringBuilder as a parameter. However, I was trying to keep consistence and use only PInvoke.User32, which only takes IntPtr as parameters. If there's any way to cast StringBuilder to IntPtr, I would be more than happy to use that, but override it again doesn't seems to be a good idea to me (maybe I was wrong). I haven't tried UIA but it definitely worth a read! Thanks! — Tide Gu, May 03 '17 at 12:28
@TideGu: Consistency is nice, correctness is better. There's nothing sacred about `PInvoke.User32`. In particular, the real function is `SendMessage`. Managed wrappers around it are just that: managed wrappers. Have one, have a thousand, as long as it makes calling unmanaged code painless (and `StringBuilder` is infinitely more painless than passing pointers around). You can even declare it as a private method if you don't want to expose the rest of the world to this version. — Jeroen Mostert, May 03 '17 at 12:35
@Ðаn Thanks. I always Google before ask, but this time I can't find anything useful. Do you have any idea on what the issue is? Especially it fails on Debug but success on Release? — Tide Gu, May 03 '17 at 12:40
@JeroenMostert Thanks for encouraging me wrap another override, and I will do. But if you can help me to spot the issues from the code? It doesn't make sense to fail it on Debug but not Release at all :( — Tide Gu, May 03 '17 at 12:43
@TideGu: Hans Passant has already explained it's heap corruption. It is, in fact, very common to see problems only in Debug and not Release, or vice versa, depending on what you're getting wrong, because memory layout and the checks performed by the marshaler and the heap allocator itself are different between modes. This is why you typically want to avoid manually allocating buffers altogether -- it's too easy to get wrong. — Jeroen Mostert, May 03 '17 at 12:50

score 3 · Accepted Answer · answered May 03 '17 at 12:43

3

IntPtr iptr = Marshal.AllocHGlobal(buff.Length);

Wrong size, you need buff.Length * sizeof(char). Twice as much as it allocates now. As written, the code corrupts the same heap that the OS uses, anything can happen next. An AVE is a normal and happy outcome, but not guaranteed.

answered May 03 '17 at 12:43

Hans Passant

922,412
146
1,693
2,536

Thanks! That's perfectly solved the issue! I was focusing on the value of `hWnd` itself but never thought it could be overflow! Even worse, I had few experience in Marshalling, and many other posts were `AllocHGlobal `-ing bytes which doesn't need a multiplier, so I haven't thought about the size. Many thanks for your great help! – Tide Gu May 03 '17 at 12:46

What's wrong with this unmanaged code in C#?

1 Answers1

Linked