dotnet / dotnet/winforms

Should we validate pointers more/better in WndProc

Open
#3,133 2 comments 0 reactions 1 assignee Claimed by @JeremyKuhne View on GitHub
area-Interop design-discussion
Dominant language
C#
Stars
4.9k
Forks
1.1k
Avg merge
20h 23m
Merged PRs (30d)
103

Description

**Is your feature request related to a problem? Please describe.**

We've turned a lot of `WndProc` related code into pointers. This is great, as it helps with marshalling etc., is so much cleaner and avoids allocations.

However, I've noticed this may lead to problems.
Consider `ListView.WmReflectNotify` which handles `WM.REFLECT | WM.NOTIFY`, as below.

```cs
private unsafe void WmReflectNotify(ref Message m)
{
User32.NMHDR* nmhdr = (User32.NMHDR*)m.LParam;

switch (nmhdr->code)
{
case (int)NM.CUSTOMDRAW:
CustomDraw(ref m);
break;

case (int)LVN.BEGINLABELEDITW:
{
NMLVDISPINFO* dispInfo = (NMLVDISPINFO*)m.LParam;
LabelEditEventArgs e = new LabelEditEventArgs(dispInfo->item.iItem);
...
}

...
}
}
```

The code `switch (nmhdr->code)` can now throw `AccessViolationException` (I think it used to throw NRE in .NET Framework)

The code `dispInfo->item.iItem` can now also throw `AccessViolationException` (I think it used to throw NRE in .NET Framework)

Should we harden these against crashes/null dereferences? I don't really see how the native code in comctl32 would send `NULL` pointers but I obviously don't have the code. Also theres a chance that people could pass bogus data into the method.

**Describe the solution you'd like and alternatives you've considered**
Maybe validate things, how? Early returns? Throwing - probably not.

**Will this feature affect UI controls?**
No

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.