Simplify and improve integer overflow checks in Interop#21732
Conversation
- Delete unnecessary CheckStringLength calls for result of string.Length. Managed strings are guaranteed to be under 2GB bytes, so these checks were unnecessary. - Add `checked(...)` around buffer size computations that may hit potential integer overflow. It does not look like any of these would cause a bug that would lead to buffer overrun, but it is better to catch these early.
|
cc @benaadams Related to #21729 (review) |
|
@dotnet-bot test Ubuntu x64 Checked CoreFX Tests please |
|
|
||
| // marshal the object as Ansi string (UnmanagedType.LPStr) | ||
| int allocSize = (pManagedHome.Capacity * Marshal.SystemMaxDBCSCharSize) + 4; | ||
| int allocSize = checked((pManagedHome.Capacity * Marshal.SystemMaxDBCSCharSize) + 4); |
There was a problem hiding this comment.
Am I to assume the + 4 is because of BSTR semantics? If so can we doc that as well. I don't fully understand why that is true based on the comment above though. What am I missing?
There was a problem hiding this comment.
+ 4is because ofBSTRsemantics?
Looking at the code that follows, my guess that this is some kind of compat quirk. There was probably some broken .NET Framework app with buffer overrun, and we have made it "work" by allocating bigger buffer than strictly necessary and filling that buffer with 3 extra null terminators. I am just speculating. I was not able to trace down where this came from in the source control history.
…lr#21732) - Delete unnecessary CheckStringLength calls for result of string.Length. Managed strings are guaranteed to be under 2GB bytes, so these checks were unnecessary. - Add `checked(...)` around buffer size computations that may hit potential integer overflow. It does not look like any of these would cause a bug that would lead to buffer overrun, but it is better to catch these early. Commit migrated from dotnet/coreclr@a5b1c68
3 participants
checked(...)around buffer size computations that may hit potential integer overflow. It does not look like any of these would cause a bug that would lead to buffer overrun, but it is better to catch these early.