Consistenly assign Broadlink.devices keys with macAddress.toString('hex')

[octavifs]

Hi Cameron,

I'm a heavy user of homebridge-broadlink-rm, with 4 of those devices at home, and I had been struggling with auto device discoverability for a while. Your library always found 3 of my 4 devices, and not necessarily the same 3. Initially, I thought it was the broadlink hardware entering locked mode, but when using python-broadlink I always managed to find the 4 of them. So, I finally spent some time today to dig into it, and it seems that I found out the root cause.

Essentially, there is an expectation on the onMessage() handler that the key for Broadlink.devices will be generated as macAddress.toString('hex'), but during the addDevice() method we directly assign the buffer value as the key: this.devices[macAddress] = device

That, in itself, seems innocent enough. The check to prevent re-adding existing devices is wasted, since macAddress.toString('hex') != String(macAddress), but the resulting Device object is the same.

Now, here's the interesting part, String(macAddress) can return the same string, for different buffer values. And that's exactly what was happening in my case. I had 2 devices, with 2 different mac addresses, that resulted in the same value when stringified. This script demonstrates the bug:

const assert = require('node:assert').strict;

let macAStr = 'ec0bae9e9d8b'
let macBStr = 'ec0bae9eb0b4'

let macABuff = Buffer.from(macAStr, 'hex')
let macBBuff = Buffer.from(macBStr, 'hex')

let devices = {}
devices[macABuff] = macABuff
devices[macBBuff] = macBBuff

assert.strictEqual(macABuff, macABuff, "Buffer A value matches itself")
assert.strictEqual(macBBuff, macBBuff, "Buffer B value matches itself")
assert.notStrictEqual(macABuff, macBBuff, "Buffer A and B values are different")
assert.notStrictEqual(macABuff.toString('hex'), macBBuff.toString('hex'), "Buffer A and B .toString('hex') values are different")
assert.strictEqual(String(macABuff), String(macBBuff), "String(Buffer A) and String(Buffer B) values are the same!!!")
assert.strictEqual(Object.keys(devices).length, 1, "devices Object has only 1 key!")
assert.strictEqual(Object.keys(devices)[0], String(macABuff), "devices key matches String(macABuff)")

The PR simply ensures that broadlink.devices are assigned with macAddress.toString('hex'). That ensures the serialization is safe, and also prevents from re-adding devices that were already discovered. I've been trying this fix on my homebridge installation and has been working like a charm so far. Would be very grateful if you could merge it in, and include it in a future release of homebridge-broadlink-rm. Let me know if you need any modifications on the PR, add tests, etc.

Thank you very much for your software!

[banboobee]

I believe your observation is reasonable. Implicit conversion to index the devices uses default encoding of utf-8, and potentially becomes identical for different mac addresses. Thus, need to specify hex explicitly.

[kiwi-cam]

Approved for BETA release

[kiwi-cam]

I'll look at a new BETA release in the next day or two

[octavifs]

Thanks! Much appreciated!

[banboobee]

homebridge-broadlink-rm-pro@4.4.16-beta.2 doesn't refer this beta.

$ sudo npm install -g homebridge-broadlink-rm-pro@4.4.16-beta.2
$ cd homebridge-broadlink-rm-pro
$ grep -i version node_modules/kiwicam-broadlinkjs-rm/package.json
"version": "0.9.19",