Undefined behaviour in Bond.cpp

[djcsdy]

I may be being an idiot here, but it looks like there's a rare undefined behaviour in Bond::assignFlowToBondedPath:

https://github.com/zerotier/ZeroTierOne/blob/39f3f5b2d95fe1e393954623b0943189441df187/node/Bond.cpp#L582-L583

bestQuality is initialized to 0, and nextBestQualIdx is initialized to a value which is outside the bounds of _realIdxMap, with the intention of reassigning it later before it is used.

https://github.com/zerotier/ZeroTierOne/blob/39f3f5b2d95fe1e393954623b0943189441df187/node/Bond.cpp#L596-L598

A new random number is chosen for each iteration through this loop.

https://github.com/zerotier/ZeroTierOne/blob/39f3f5b2d95fe1e393954623b0943189441df187/node/Bond.cpp#L600-L605

When we are assigning a flow for the first time, the random number is combined with the incrementing offset to select a link index. Due to the randomness, this might be the same index each time. For a bond with two links, there is a 50% chance that the same link will be checked twice, and the other link will never be checked. This might be by design but I suspect the intention here was to use the same random number for each iteration and check each link at most once.

(Also I don't think abs is needed here, since there are no negative numbers involved.)

https://github.com/zerotier/ZeroTierOne/blob/39f3f5b2d95fe1e393954623b0943189441df187/node/Bond.cpp#L614-L618

If the link doesn't meet the quality requirements, then nextBestQualIdx is assigned only if relativeQuality > bestQuality.

Since bestQuality is initialized to zero, If all the links tested have relativeQuality == 0.0, then nextBestQualIdx is never assigned in the loop. Due to the randomness, it's possible some links will never be tested and therefore it's possible for this to happen if even one link has relativeQuality == 0.0.

https://github.com/zerotier/ZeroTierOne/blob/39f3f5b2d95fe1e393954623b0943189441df187/node/Bond.cpp#L627-L632

Finally, if none of the tested links meet the quality requirements, _realIdxMap[nextBestQualIdx] is dereferenced. If nextBestQualIdx wasn't assigned in the loop, then this will be out of bounds.

https://github.com/zerotier/ZeroTierOne/blob/39f3f5b2d95fe1e393954623b0943189441df187/node/Bond.cpp#L1274-L1293

Judging by this code that assigns relativeQuality, it's quite possible for relativeQuality to be zero, causing undefined behaviour as described above.

[joseph-henry]

Thanks for reporting this. I'll take a look. If you have a PR ready I can possibly merge it otherwise I can issue a patch. Up to you.

[djcsdy]

I have a fix for this but I didn't raise a PR only because I'm not exactly sure what the intended behaviour is.

I'll submit a PR anyway and you can decide what you want to do :-).