Skip to content

Fix UDP socket binding for WhoIs/IAm broadcast on Linux#157

Open
imurasawa wants to merge 1 commit into
ela-compil:masterfrom
imurasawa:fix/linux-udp-socket-binding
Open

Fix UDP socket binding for WhoIs/IAm broadcast on Linux#157
imurasawa wants to merge 1 commit into
ela-compil:masterfrom
imurasawa:fix/linux-udp-socket-binding

Conversation

@imurasawa

Copy link
Copy Markdown

Summary

This PR fixes an issue where WhoIs broadcast packets are not received on Linux (.NET 8 / Raspberry Pi 4), causing IAm responses to never be sent.

Related issues: #88, #48, #59, #93


Root Cause

On Windows, broadcast packets are forwarded to all addresses bound within the subnet. On Linux, broadcast packets are only delivered to sockets explicitly bound to the broadcast address or 0.0.0.0.

As a result:

  • If _sharedConn is bound to a specific IP, WhoIs broadcast packets are never received
  • new UdpClient(ep) binds without ReuseAddress, causing a second bind to the same port to fail on Linux, which results in a crash when initializing _exclusiveConn

Changes

File: Transport/BacnetIpUdpProtocolTransport.cs

Fix 1: Line 100 — Remove localEndpoint override for _sharedConn

_sharedConn must always be bound to IPAddress.Any (0.0.0.0) in order to receive broadcast packets on Linux. Lines 97–99 are unchanged. Only the if statement on line 100 is removed.

                _sharedConn = new UdpClient { ExclusiveAddressUse = false };
                _sharedConn.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true);
                var ep = new IPEndPoint(IPAddress.Any, SharedPort);
-               if (!string.IsNullOrEmpty(_localEndpoint)) ep = new IPEndPoint(IPAddress.Parse(_localEndpoint), SharedPort);
                DisableConnReset(_sharedConn);

Fix 2: Line 112 — Add ReuseAddress for first open of _exclusiveConn

-               _exclusiveConn = new UdpClient(ep);
+               _exclusiveConn = new UdpClient { ExclusiveAddressUse = false };
+               _exclusiveConn.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true);
+               _exclusiveConn.Client.Bind(ep);

Fix 3: Lines 121–124 — Add ReuseAddress for re-open of _exclusiveConn

-               _exclusiveConn = new UdpClient(ep)
-               {
-                   EnableBroadcast = true
-               };
+               _exclusiveConn = new UdpClient { ExclusiveAddressUse = false };
+               _exclusiveConn.Client.SetSocketOption(SocketOptionLevel.Socket, SocketOptionName.ReuseAddress, true);
+               _exclusiveConn.EnableBroadcast = true;
+               _exclusiveConn.Client.Bind(ep);

Recommended Configuration

After this fix, the following combination is recommended on Linux:

  • useExclusivePort = false
  • localEndpointIp = the IP address of the BACnet network interface (e.g. 192.168.10.12)

With this combination:

  • _sharedConn (bound to 0.0.0.0) handles WhoIs broadcast reception
  • _exclusiveConn (bound to specific IP) handles unicast send/receive
  • No duplicate IAm responses
  • In multi-NIC environments, GetAddressDefaultInterface() is bypassed entirely and the broadcast address is calculated correctly based on the specified IP

Relationship to PR #100

PR #100 proposed changing unicastAddresses.Single() to unicastAddresses.First() to address the issue where GetAddressDefaultInterface() returns null in multi-NIC environments, causing 255.255.255.255 (which is not compliant with the BACnet standard) to be used as the broadcast address. However, the concern that "which NIC will be selected is non-deterministic" was never resolved, and the PR has not been merged.

This PR avoids the problem entirely by bypassing GetAddressDefaultInterface() when localEndpointIp is explicitly specified. This approach works correctly in both of the following cases:

  • A single NIC with multiple IP addresses configured
  • A second or later NIC used for BACnet in a multi-NIC environment

Test Results

Before this fix (original code):

useExclusivePort localEndpointIp Single NIC Multiple NICs
false 0.0.0.0 or empty Crash ❌ Not tested ※1
false Specific IP Crash ❌ Not tested ※1
true 0.0.0.0 or empty IAm once ✅ Not working ❌
true Specific IP No response to WhoIs ❌ No response to WhoIs ❌

After this fix (this PR):

useExclusivePort localEndpointIp Single NIC Multiple NICs
false 0.0.0.0 or empty Duplicate IAm ❌ Not working ❌
false Specific IP IAm once ✅ IAm once ✅
true 0.0.0.0 or empty IAm once ✅ Not working ❌
true Specific IP No response to WhoIs ❌ No response to WhoIs ❌

Legend:

  • Crash: Exception thrown during initialization; BACnet service does not start
  • Not working: Service starts, but no IAm is sent and no response to WhoIs. Device cannot be detected by YABE even if YABE is started first
  • No response to WhoIs: IAm is sent on startup (device can be detected if YABE is started first), but does not respond to subsequent WhoIs requests
  • Duplicate IAm: Two IAm responses are sent for a single WhoIs request
  • ※1: Not tested on original code with multiple NICs. However, given the structure of the original code, it is highly unlikely to work correctly

Test Environment

This fix was developed and tested on .NET 8 / Linux (Raspberry Pi 4). We are using .NET 8 as recommended in response to the memory leak reported in #89. The fix itself does not depend on .NET 8 and should be applicable to net48 / netstandard2.0 as well, but has not been verified in those environments.

Windows and Mac are untested. For potential impact of ReuseAddress on Windows, see dotnet/runtime#83525.


Not Included in This PR

@imurasawa

Copy link
Copy Markdown
Author

I noticed that this file is the only one in the repository where CRLF and LF line endings are mixed.

Scanning all .cs files in the repository:

  • LF only: almost all files
  • CRLF only: BACnetClient.cs
  • Mixed (CRLF + LF): BacnetIpUdpProtocolTransport.cs only

This was introduced by PR #95, where shaggygi inadvertently left the header as LF while converting the rest of the file to CRLF.

You can verify with PowerShell:

Get-ChildItem -Recurse -Filter *.cs | ForEach-Object {
    $bytes = [System.IO.File]::ReadAllBytes($_.FullName)
    $text = [System.Text.Encoding]::UTF8.GetString($bytes)
    $hasCRLF = $text -match "`r`n"
    $hasLFonly = $text -match "(?<!`r)`n"
    if ($hasCRLF -and $hasLFonly) {
        Write-Output $_.FullName
    }
}

Should this file be normalized to LF? Happy to submit a separate PR if desired.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant