arp output parsing for SunOS/illumos is wrong resulting in only lo0 being used

sjorge commented 2 years ago

Analysis

Took me a bit to track down but I noticed ciao wasn't working on illumos, but there was some SunOS handling and given illumos forked from SunOS a few years ago generally we're still rather compatible.

I spend a few hours hunting it down and it fumbles here: https://github.com/homebridge/ciao/blob/3ce9b350117206ee80234d6dbed9bc54003deff1/src/NetworkManager.ts#L761

The offset for illumos (and I verified via a friend who has access to an Oracle Solaris 11 box) is wrong, it should be 0. If I monkey patch the offset from 2 -> 0 in the node_modules generate .js file it works as expected.

Expected Behavior

CIAO should find and use the correct interface(s) instead of just lo0.

Steps To Reproduce

In my case I am using node-red-contrib-homekit-bridged with the advertiser set to CIAO, adding any Service and hitting deploy will trigger it.

Logs

Snippet from the node-red output with DEBUG=ciao:*, you can see it only ever selecting lo0 which is pretty useless.

  ciao:NetworkManager Created NetworkManager with options: {"excludeIpv6Only":true} +4m
  ciao:Responder [amethyst A181._hap._tcp.local.] Going to advertise service... +4m
  ciao:NetworkManager Initial networks [lo0] ignoring [amethyst0] +92ms
  ciao:CiaoService [amethyst A181] Rebuilding service records... +4m
  ciao:Prober Starting to probe for 'amethyst A181._hap._tcp.local.'... +4m
  ciao:Prober Sending prober query number 1 for 'amethyst A181._hap._tcp.local.'... +172ms
  ciao:Prober Sending prober query number 2 for 'amethyst A181._hap._tcp.local.'... +255ms
  ciao:Prober Sending prober query number 3 for 'amethyst A181._hap._tcp.local.'... +252ms
  ciao:Prober Probing for 'amethyst A181._hap._tcp.local.' finished successfully +253ms
  ciao:Announcer [amethyst A181._hap._tcp.local.] Sending announcement for service +4m
  ciao:CiaoService [amethyst A181] Rebuilding service records... +934ms
  ciao:Announcer [amethyst A181._hap._tcp.local.] Sending announcement number 1 +2ms
  ciao:Announcer [amethyst A181._hap._tcp.local.] Sending announcement number 2 +1s
  ciao:Announcer [amethyst A181._hap._tcp.local.] Sending announcement number 3 +2s
  ciao:CiaoService [amethyst A181] Updating txt record... +19s
  ciao:CiaoService [amethyst A181] Rebuilding service records... +1ms
  ciao:Responder [amethyst A181._hap._tcp.local.] Updating 2 record(s) for given service! +20s

Configuration

n/a

Environment

OS: OmniOS r151040 (illumos)
Software: node-red + node-red-contrib-homekit-bridged
Node: v16.14.0
npm: 8.3.1

Process Supervisor

not applicable

Additional Context

arp -a -n output from an illumos based distro:

root@amethyst:~# arp -a -n
Net to Media Table: IPv4
Device   IP Address               Mask      Flags      Phys Addr
------ -------------------- --------------- -------- ---------------
amethyst0 10.23.12.1           255.255.255.255          00:25:90:f1:55:06
amethyst0 10.23.10.1           255.255.255.255          00:00:5e:00:01:0a

Output from on an Oracle Solaris system:

af@solaris:~$ arp -an
Net to Media Table: IPv4
Device   IP Address               Mask      Flags      Phys Addr
------ -------------------- --------------- -------- ---------------
net0   172.27.10.254        255.255.255.255          02:08:20:1d:02:93
net0   172.27.10.199        255.255.255.255 SPLA     02:08:20:4b:63:b7

Output for as far as I could tell has not deviated between Oracle Solaris and illumos (both return a uname of SunOS) so that's good knows as fixing one will also fix the other.

I was able to verify this my monkey-patching the offset from 2 to 0 and everything is working (include control of all my devices via home.app)

The debug output now also states that the correct amethyst0 interface is used.

  ciao:NetworkManager Created NetworkManager with options: {"excludeIpv6Only":true} +0ms
  ciao:Responder [amethyst A181._hap._tcp.local.] Going to advertise service... +0ms
  ciao:NetworkManager Initial networks [lo0, amethyst0] ignoring [] +1s
  ciao:CiaoService [amethyst A181] Rebuilding service records... +0ms
  ciao:Prober Starting to probe for 'amethyst A181._hap._tcp.local.'... +0ms
  ciao:Prober Sending prober query number 1 for 'amethyst A181._hap._tcp.local.'... +209ms
  ciao:Prober Sending prober query number 2 for 'amethyst A181._hap._tcp.local.'... +1s
  ciao:Prober Sending prober query number 3 for 'amethyst A181._hap._tcp.local.'... +596ms
  ciao:Prober Probing for 'amethyst A181._hap._tcp.local.' finished successfully +438ms
  ciao:Announcer [amethyst A181._hap._tcp.local.] Sending announcement for service +0ms
  ciao:CiaoService [amethyst A181] Rebuilding service records... +2s
  ciao:Announcer [amethyst A181._hap._tcp.local.] Sending announcement number 1 +26ms
  ciao:CiaoService [amethyst A181] Updating txt record... +712ms
  ciao:CiaoService [amethyst A181] Rebuilding service records... +3ms

I guess a properly fix this getOpenBSD_SUNOS_NetworkInterfaces() would need to be split into a OpenBSd and SunOS variant, or the offset should somehow be toggled between 2 and 0 depending on which one we are. But my typescript knowledge was not sufficient to actually do this and get it to compile :(

sjorge commented 2 years ago

diff --git a/src/NetworkManager.ts b/src/NetworkManager.ts
index abe9812..a7083c3 100644
--- a/src/NetworkManager.ts
+++ b/src/NetworkManager.ts
@@ -754,11 +754,12 @@ export class NetworkManager extends EventEmitter {
           return;
         }

+        const interfaceArrayOffset = os.platform() === "sunos" ? 0 : 2;
         const lines = stdout.split(os.EOL);
         const names: InterfaceName[] = [];

         for (let i = 1; i < lines.length - 1; i++) {
-          const interfaceName = lines[i].trim().split(NetworkManager.SPACE_PATTERN)[2];
+          const interfaceName = lines[i].trim().split(NetworkManager.SPACE_PATTERN)[interfaceArrayOffset];
           if (!interfaceName) {
             debug(`${os.platform()}: Failed to read interface name from line ${i}: '${lines[i]}'`);
             continue;

I think this should do the trick, but I can't get tests or tsc to work at all

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

sjorge commented 2 years ago

This is still an issue, a PR has also been opened

sjorge commented 2 years ago

PR got merge into beta-1.1.5 branch, closing the issue.

homebridge / ciao