Bug #264

Confusion between signed and unsigned 16bit short address

Added by Philipp Buluschek about 3 years ago. Updated about 3 years ago.

Status:Closed Start date:10/23/2014
Priority:High Due date:
Assignee:Stefano Lenzi % Done:

0%

Category:zigbee.basedriver Spent time: -
Target version:org.aaloa.zb4osgi.zigbee.basedriver-0.8.0
Has a patch:No Has license agreement signed:No

Description

In ZigBeeDeviceImpl there is a check for the correct short address before notifying listeners

public void notify(AF_INCOMING_MSG msg) {
...
if (msg.getSrcAddr() != node.getNetworkAddress())

Here the left value is a short and thus is in [-32768 .. 32767], the right value is an int, which comes from ZToolAddress16 in which DoubleByte.get16BitValue() gets called. This is implemented as
public int get16BitValue() {
return (this.msb << 8) + this.lsb;
}

which will return an unsigned value in [0..65535].

As far as I can tell, this comparison will simply discard all AF messages for devices with a short address above 32767 (0x7FF) instead of notifying the cluster listeners.

Possible fix:
if ((msg.getSrcAddr() & 0xFFFF) != node.getNetworkAddress())
or
if (msg.getSrcAddr() != (short)node.getNetworkAddress())

PS: In this same method ( ZigBeeDeviceImpl.notify(AF_INCOMING_MSG msg) ) why are the consumers notified in any case, but the listeners only if the network addresses match?


Related issues

related to Bug #269: Check network address before notifying consumers Rejected 11/03/2014
blocked by Improvement #271: Avoid to set Network Address to negative number Closed 11/05/2014

Associated revisions

Revision 1074
Added by Stefano Lenzi about 3 years ago

Applied fixes for handling dispatching of messages to ZigBeeDevice with a network address greater then 32767 ( refs #264 )

Revision 1076
Added by Stefano Lenzi about 3 years ago

Consuming messages only when network address and endpoint match ( refs #264 )
Fixed typo in AFMessageListner javadoc

Revision 1077
Added by Stefano Lenzi about 3 years ago

Added some comments to the source code for changing the code if it does not match all the use cases ( refs #264 )

Revision 1082
Added by Stefano Lenzi about 3 years ago

Added JUnit for verify and catch the "unsign/sign" bug for message dispatching ( refs #264 )

Revision 1085
Added by Stefano Lenzi about 3 years ago

Added another Test Case that enable catching if AF_INCOMING_MSG are consumed by the wrong AFMessageConsumer ( refs #264 )

Revision 1086
Added by Stefano Lenzi about 3 years ago

Improved log messages ( refs #264 )

Revision 1087
Added by Stefano Lenzi about 3 years ago

Avoding to store negative network address ( refs #264 )

History

#1 Updated by Stefano Lenzi about 3 years ago

  • Category set to zigbee.basedriver
  • Status changed from New to In Progress
  • Assignee set to Stefano Lenzi
  • Target version set to org.aaloa.zb4osgi.zigbee.basedriver-1.X

Thank you for finding this glitch, I'm afraid that this "signed/unsigned" may be present in other part of the code.

Regarding your question I have to look back at the code...

#2 Updated by Stefano Lenzi about 3 years ago

  • Status changed from In Progress to Resolved

Hi Philipp,

I have just applied the fixes for the "sign" issue.

Regarding the question about the different notification of the messages between the AFMessageConsumer and ClusterListener, the answer is:
  • The AFMessageConsumer is notified in advance and before the ClusterListener, and if it consume the message the message is not notified to anyone else. In the current implementation, ZB4O uses the AFMessageConsumer for synchronizing the Command Request / Command Response which requires the use of the network, thus it will waste a lot of resource to perform an active waiting for the answer anytime a Command Request is sent.
  • The ClusterListener is use for handling message that are converted from ZToolPacket to Cluster and it is designed for providing the Cluster to the ZB4O application, while the AFMessageCluster is used only internally by the ZigBee Base Driver

Please close the issue if it is clear

#3 Updated by Philipp Buluschek about 3 years ago

Hello Stefano
Actually the question was, why the network address is not checked before notifying consumers, but it is checked before notifying listeners.

In my understanding, all (consumer and listeners) should only be called if the network address matches, so the check should be done first in ZigBeeDeviceImpl.notify().

Additionally, the only type of consumer that is registered is WaitForClusterResponse in which (.consume() ) the network address is not checked. So in theory the following bug could happen (especially because transaction IDs are disabled)
ZigBeeDevice1 sends request for something to its device
ZigBeeDevice2 sends request for the same thing to its own device
Answer from 1 gets consumed by consumer of 2...

#4 Updated by Philipp Buluschek about 3 years ago

Reviewed the changes for the original problem (signed vs unsigned) and it looks good.

#5 Updated by Stefano Lenzi about 3 years ago

Yes I think we can control EndPoint and NwkAdress before consuming the message, because the idea is to consume the response of Cluster and as far as I know there is no use case where a Cluster request is unicast and the answer i broadcast or groupcast, if we find such case then we have to revert the code.

I think we can close this issue. Would you mind to close it?

#6 Updated by Philipp Buluschek about 3 years ago

  • Status changed from Resolved to Closed

Good point about broadcasts. I opened #269 to handle this.

Closing this.

#7 Updated by Stefano Lenzi about 3 years ago

  • Has a patch changed from Yes to No

#8 Updated by Stefano Lenzi about 3 years ago

  • Target version changed from org.aaloa.zb4osgi.zigbee.basedriver-1.X to org.aaloa.zb4osgi.zigbee.basedriver-0.8.0

Also available in: Atom PDF