Skip to content

Streamline SystemIPGlobalProperties#125002

Open
pentp wants to merge 2 commits intodotnet:mainfrom
pentp:net-info
Open

Streamline SystemIPGlobalProperties#125002
pentp wants to merge 2 commits intodotnet:mainfrom
pentp:net-info

Conversation

@pentp
Copy link
Contributor

@pentp pentp commented Feb 28, 2026

  • Remove multiple layers of allocations from GetActiveTcpConnections/GetActiveTcpListeners.
  • Use native API filtering for listeners and entry state filtering for connections.
  • Adjust interop structures to more closely match the native declaration.
  • Minimize unsafe pointer manipulation and use safe spans instead for enumerating entries.

Copilot AI review requested due to automatic review settings February 28, 2026 05:00
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Feb 28, 2026
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to this area: @karelz, @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR streamlines Windows SystemIPGlobalProperties TCP/UDP enumeration by reducing managed allocations and shifting more filtering/shape-matching into the native interop layer.

Changes:

  • Switch TCP/UDP enumeration to GetExtendedTcpTable/GetExtendedUdpTable, using native table-class filtering for listener queries.
  • Reshape IpHlpApi interop table/row structs to more closely match native layouts and expose LocalEndPoint/RemoteEndPoint helpers.
  • Replace byte-slicing parsing with typed span enumeration over table entries.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.

File Description
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SystemTcpConnection.cs Simplifies connection info construction by consuming new interop State/endpoint helpers.
src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SystemIPGlobalProperties.cs Reworks TCP/UDP enumeration to use extended tables, native filtering, and span-based iteration.
src/libraries/Common/src/Interop/Windows/IpHlpApi/Interop.NetworkInformation.cs Updates interop struct layouts and adds endpoint helper properties; removes unused legacy P/Invokes.
Comments suppressed due to low confidence (1)

src/libraries/System.Net.NetworkInformation/src/System/Net/NetworkInformation/SystemIPGlobalProperties.cs:198

  • The comment for GetActiveUdpListeners is now inaccurate: the IPv4 path was switched from GetUdpTable to GetExtendedUdpTable. Please update the comment to reference the correct native API.
        /// Gets the active UDP listeners. Uses the native GetUdpTable API.
        public override unsafe IPEndPoint[] GetActiveUdpListeners()
        {

@stephentoub
Copy link
Member

🤖 Copilot Code Review — PR #125002

Holistic Assessment

Motivation: Well-justified. The old code had unnecessary intermediate allocations (a full List<SystemTcpConnectionInformation> for all entries, then filtering), used GetTcpTable/GetUdpTable when the more flexible GetExtendedTcpTable/GetExtendedUdpTable could filter at the OS level, and had interop struct definitions that used individual bytes for what the Windows API defines as DWORDs.

Approach: Clean and correct. Consolidating onto GetExtendedTcpTable/GetExtendedUdpTable, using typed ReadOnlySpan<T> instead of byte-slicing, moving port conversion into struct properties, and requesting only listeners from the OS when that's all that's needed — all good improvements.

Summary: ✅ LGTM. This is a well-executed refactoring that reduces allocations, eliminates dead code, and modernizes the interop layer. Struct layouts were verified against the Windows API, port endianness conversion is correct, and the filtering logic preserves existing behavior. Two minor suggestions below.


Detailed Findings

✅ Struct Layout Correctness — All struct sizes match the Windows API

Verified all four row struct sizes against the Windows API definitions:

  • MibTcpRow: 5 × uint = 20 bytes → matches MIB_TCPROW
  • MibTcp6RowOwnerPid: 16 + 4 + 4 + 16 + 4 + 4 + 4 + 4 = 56 bytes → matches MIB_TCP6ROW_OWNER_PID
  • MibUdpRow: 4 + 4 = 8 bytes → matches MIB_UDPROW
  • MibUdp6RowOwnerPid: 16 + 4 + 4 + 4 = 28 bytes → matches MIB_UDP6ROW_OWNER_PID

No padding concerns: all table structs have uint NumEntries followed by a row struct with 4-byte alignment, so FirstEntry starts at offset 4 with no gap.

✅ Port Endianness Conversion — Verified correct

The new BinaryPrimitives.ReverseEndianness((ushort)LocalPort) is equivalent to the old localPort1 << 8 | localPort2. Verified: for port 443 (0x01BB in network byte order), LocalPort on little-endian reads as 0xXXXXBB01, (ushort) gives 0xBB01, ReverseEndianness gives 0x01BB = 443. ✓

MihaZupan's endianness question is addressed by pentp's response — this is Windows-only code, and Windows only runs on little-endian architectures.

✅ Filtering Logic — Correct and more efficient

  • GetActiveTcpListeners() now passes (null, list), causing the IPv4 path to use TcpTableBasicListener and the IPv6 path to use TcpTableOwnerPidListener — the OS returns only listeners, avoiding unnecessary data transfer and processing.
  • GetActiveTcpConnections() passes (list, null), using TcpTableBasicAll/TcpTableOwnerPidAll and filtering inline with null-conditional operators (connections?.Add(...), listening?.Add(...)) — no NRE risk.
  • The old remotePort = 0 special case for Listen entries is safely removed since Listen entries no longer flow through SystemTcpConnectionInformation.

✅ No Public API Changes — All modifications are internal

No ref assembly changes. All renamed fields, new properties, and removed P/Invokes are internal.

💡 Minor Optimization Opportunity — TcpTableBasicConnections

When GetActiveTcpConnections is the caller (connections is not null, listening is null), the code uses TcpTableBasicAll which returns all entries including listeners (which are then silently discarded). Using TcpTableBasicConnections (enum value 1) / TcpTableOwnerPidConnections (enum value 4) would avoid fetching listener entries entirely. This would require a slightly more nuanced table-class selection, e.g.:

connections is null ? TcpTableBasicListener :
listening is null ? TcpTableBasicConnections :
TcpTableBasicAll

Not blocking — the current approach is functionally correct and matches the old behavior. But it's a natural follow-up optimization given the infrastructure is already in place.

💡 Existing Copilot Review Comments — Addressing accuracy

The automated Copilot review on this PR flagged Debug.Assert vs runtime bounds checking. This is not a real concern: the old code had the same implicit trust of numberOfEntries, and trusting OS API return values for buffer sizes that the OS itself provided is standard practice in this codebase. The Debug.Assert is a good addition for catching issues during development.

The Copilot review also mentioned connections! causing potential NRE, but the code actually uses connections?.Add(...) (null-conditional), so there is no NRE risk.

@pentp
Copy link
Contributor Author

pentp commented Mar 5, 2026

Regarding TcpTableBasicConnections/TcpTableOwnerPidConnections for filtering - I checked and it only returns connections in Connected state, so that's not usable unfortunately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-System.Net community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants