Fix memory overwrites in SystemNative_GetNetworkInterfaces#125022
Fix memory overwrites in SystemNative_GetNetworkInterfaces#125022gwr wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @karelz, @dotnet/ncl |
There was a problem hiding this comment.
Pull request overview
This PR fixes a potential buffer overlap in SystemNative_GetNetworkInterfaces by ensuring the native allocation always reserves space for both the returned NetworkInterfaceInfo array and the IpAddressInfo array, even when getifaddrs returns only IPv4/IPv6 entries (so count == ip4count + ip6count).
Changes:
- Make the allocation size always account for
count + ip4count + ip6countentries to prevent overwriting the interface array. - Set
addressListto start immediately afterNetworkInterfaceInfo[count](instead of a computed offset that can become 0). - Remove the Android-only conditional sizing logic and replace it with a platform-agnostic approach.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/native/libs/System.Native/pal_interfaceaddresses.c:365
- On allocation failure, the getifaddrs() result in
headis not freed before returning. This leaks the ifaddrs list in the OOM path; please callfreeifaddrs(head)before settingerrno/returning (and keepheadvalid for that call).
void * memoryBlock = calloc((size_t)entriesCount, sizeof(NetworkInterfaceInfo));
if (memoryBlock == NULL)
{
errno = ENOMEM;
return -1;
}
|
Fixed the assert, squash. |
Not only Android might see count = ip4count + ip6count, which would lead to overwriting the NetworkInterfaceInfo this is meant to return. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
An earlier version didn't build. Someone please "kick" the CI stuff to run again. |
| // Make no assumptions about how many ip4count + ip6count there may be. | ||
| // This does assume sizeof(NetworkInterfaceInfo) >= sizeof(IpAddressInfo) | ||
| // which is checked in an assert above this function. | ||
|
|
||
| int entriesCount = count + ip4count + ip6count; |
There was a problem hiding this comment.
This part is curious. The count is number of entries returned by getifaddrs. It does not seem like single entry would have more addresses so I'm wondering how we would get more than count addresses @gwr
There was a problem hiding this comment.
In the failure happens where (in my example) we have two interfaces and three addresses.
I've attached a test program I used to demonstrate and debug, along with it's output.
test-getif.cs
test-getif-out.txt
Where the non-Android path was starting writing addresses at (entriesCount - ip4count - ip6count) that would overwrite the first interface (NetworkInterfaceInfo*) that was already written to the combined output buffer. I could recreate and show you a gdb log if you like.
Not only Android might see count = ip4count + ip6count, which would lead to overwriting the NetworkInterfaceInfo this is meant to return.
Something I discovered while working on #124728