Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixing resolution of broadcast address if multiple ip addresses are available #100

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixing resolution of broadcast address if multiple ip addresses are available #100

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions Transport/BacnetIpUdpProtocolTransport.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,8 +371,8 @@ private static UnicastIPAddressInformation GetAddressDefaultInterface()
.Where(a => a.Address.AddressFamily == AddressFamily.InterNetwork)
.ToArray();

return unicastAddresses.Length == 1
? unicastAddresses.Single()
return unicastAddresses.Length >= 1
? unicastAddresses.First()
: null;
Comment on lines 372 to 376
Copy link
Member

Choose a reason for hiding this comment

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

How about we remove .ToArray() and do

return unicastAddresses.FirstOrDefault()

Copy link
Member

Choose a reason for hiding this comment

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

But, this would change the original behavior. What is the issue issue with having null returned here? Is it the invalid broadcast address used later?

Copy link
Author

Choose a reason for hiding this comment

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

Exactly, when null is returned the broadcast address is resolved to 255.255.255.255 which is not allowed in BACnet.

Copy link
Author

Choose a reason for hiding this comment

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

Taking one step back, the original problem is when using 0.0.0.0 when you have multipe interfaces active or an interface in use with multiple ip's bound to it. -> In this case, it's not clear which broadcast address should be used at all?
In my opinion, there has to be a check when initializing the udp transport to bind to a dedicated ip address available on the device, if multiple options are available. In this case, the correct broadcast address will always be resolved automatically!

Copy link
Member

Choose a reason for hiding this comment

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

Ok. But if we take TcpListener for example and tell it to bound to 0.0.0.0, it wil listen on all available interfaces. I think this is the reason the original authors wanted to listen on all interfaces, not only one. As for broadcast, I'm not sure, does sending udp packet to 255.255.255.255 results in sending it via all interfaces or it doesn't work at all? If it's the later, then maybe broadcast address should always be explicitly specified and not automatically resolved.

Copy link
Author

Choose a reason for hiding this comment

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

That is correct! 0.0.0.0 will listen on all interfaces -> the resolution of the broadcast address is the only issue here.
Regarding the validity of the 255.255.255.255 broadcast address in BACnet, I'll have a detailed look on the spec.
As far as I know it is not allowed. Unfortunately, I already saw devices from big vendors relying on it, though.
-> has to be double checked before merging :-(

Will give an update on this...

Copy link
Author

Choose a reason for hiding this comment

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

A UDP broadcast packet is 'sent' to an IP address with the subnet of the broadcasting device and all 1's in the host portion. For example, if a device has an address 128.253.109.10 and a subnet mask 255.255.255.0, it can use the broadcast address 128.253.109.255. Although most networks also allow the use of the IP broadcast address 255.255.255.255, we have ruled this out for BACnet/IP because of some IP protocol stack limitations that certain implementors have encountered.

src: http://www.bacnet.org/Tutorial/BACnetIP/sld006.html

Copy link
Contributor

Choose a reason for hiding this comment

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

This bug bit us a few times. I would prefer some sort of a thrown exception if a reasonable interface can't be found (after filtering out lo). Using first available interface may not be good, as it would be non-deterministic, and might pick the wrong one

Copy link

Choose a reason for hiding this comment

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

+1 this bit me the first time I tested the library out as well.

I would prefer an exception if no suitable interface could be found. If multiple suitable interfaces are found, either pick the first one or better yet - being able to specify which one based on some sort of rule set (interface name, type, identifier etc). Or expose a public method that gets the list of suitable interfaces and accept one of them as the argument.

}

Expand Down