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.

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

Short and Long node names are created incorrectly #16

Open
alamaral opened this issue Jul 14, 2022 · 0 comments
Open

Short and Long node names are created incorrectly #16

alamaral opened this issue Jul 14, 2022 · 0 comments

Comments

@alamaral
Copy link

In the ArtNet protocol it states that both the long and short names are null-terminated, i.e. "C" style, strings. However,
the code that creates the names simply grabs the entire range of bytes from the packet and stuffs it into a String.

This has 2 effects:

  1. If someone calls getShortName or getLongName they get a string with random garbage at the end of it.
  2. The node discovery code relies on the garbage remaining the same to work.

#1 is a problem because if the code uses either name, like printing/logging ArtNetNode.toString(), the output will
contain the garbage.
#2 is a problem because in ArtNetNodeDiscovery the lastDiscovered member variable is a List. There is code in the
thread run function that calls lastDiscovered.contains(node), which invokes either equals() or hashCode() on two objects
to determine if the objects are the same. In either function, if the garbage in either name string has changed the return
value will be false, leading to the code not finding the entry, and making it looks like the node has disconnected, then
reconnected again, with a different name, even though everything else is unchanged.

The way to fix both problems is to strip off everything after the null character when parsing the ArtPollReplyPacket. Here
is a fix that works for me:

private String stripGarbage(String s) {
    int idx = s.indexOf(0);
    if (idx >= 0) {
        s = s.substring(0, idx);
    }
    return s;
}

@Override
public boolean parse(byte[] raw) {
    setData(raw);
    // Extraneous code removed:
    // Both the short and long names are null terminated strings.  When setting
    // the names we have to strip the null char and the garbage after it, otherwise
    // the names will contain that garbage:
    shortName = stripGarbage(new String(data.getByteChunk(null, 26, 17)));
    longName = stripGarbage(new String(data.getByteChunk(null, 44, 63)));

One possible optimization would be making lastDiscovered a ConcurrentHashMap, like discoveredNodes. This would
allow the code to call lastDiscovered.get(node.getIPAddress()) instead of contains(). Then, if a node with the correct IP
address is found, the nodes could be compared using equals().

This also begs the question about whether lastDiscovered, which is a simple List, needs to be thread-safe, or at least
synchronized. It is being manipulated by multiple threads, so I suspect the code should be using a thread-safe collection,
like a ConcurrentHashMap, anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant