YARP IP Theft discussion

[traversaro]

There is an unclear aspect of how the YARP port registration works, quoting @Nicogene from robotology/icub-tech-support#999 (comment):

I usually call it "IP theft", it happens when you are on a setup with more than one node, and after running a module that opens the port /foo you by accident opens the same port from another machine e.g. via yarp write /foo.

What happens is that in the nameserver the /foo port will have the IP and port number assigned to the yarp write because being on different machines no address conflict is triggered and the first /foo port is reachable only via the IP, because trying to contact via name we will contact actually the second /foo created.

This is a known issue about the ports registration in YARP, there is no check if the entry exists already because it is possible that the entry remained after the crash of one module, the only check is on the address conflict triggered by the OS.

We opened this issue to understand if this aspect can be improved, either by changing the behavior or by just documenting it better.

[traversaro]

cc @Nicogene @drdanz @lornat75 @AlexAntn @S-Dafarra

[S-Dafarra]

My proposition for this problem was to check the IP of the module opening the port. If a module wants to register a port, but that port is already registered by another address, then something is likely going wrong. I don't know whether this check should be done by the name server, or inside the open method.

This would still allow avoiding running yarp clean when a module crashes, as in the majority of the cases, after a crash, the module gets restarted in the same machine, hence with the same IP. If this is not the case, it is still possible to run yarp clean.

[traversaro]

Related code (shared by @drdanz), currently commented out:

yarp/src/libYARP_serversql/src/yarp/serversql/impl/NameServiceOnTriples.cpp

Lines 209 to 245 in c6adf01

	if (result!=-1) {
	// Hmm, we already have a registration.
	// This could be fine - maybe program crashed or was abruptly
	// terminated. Classically, that is what YARP would assume.
	// Now, let us try checking instead to see if there is a port
	// alive at the registered address. Note that this can lead
	// to delays...
	#if 0
	Contact c = query(port.c_str());
	if (c.isValid()) {
	if (gonePublic) {
	printf(" ? checking prior registration, to avoid accidental collision\n");
	Bottle cmd("[ver]"), reply;
	double timeout = 3.0;
	double pre = SystemClock::nowSystem();
	bool ok = Network::write(c,cmd,reply,true,true,timeout);
	double post = SystemClock::nowSystem();
	if (post-pre>timeout-1) {
	ok = false;
	}
	if (ok) {
	printf(" ? prior registration seems to be live! Denying new registration.\n");
	/*
	printf("Got a live one! %s - said %s\n",
	port.c_str(),
	reply.toString().c_str());
	*/
	// oops! there is a live port!
	// give back a blank query
	act.cmd.fromString("query");
	return cmdQuery(act);
	} else {
	printf(" ! prior registration seems to be no longer valid, good!\n");
	}
	}
	}
	#endif

.

[drdanz]

The code above assumes that the port is able to reply to the [ver] command (within 3 seconds of timeout).

As discussed, the limits in this approch are:

1. The port might not be a YARP port, hence it will not be able to reply to the [ver] command
2. In case the program is restarted on the same system, the system will be delayed by 3 seconds for each port that is opened, unless it was closed properly. This may lead to several minutes of delay for yarprobotinterface
3. In order to avoid 2), it would be necessary to run yarp clean, but this leads to other bugs similar to 1) and due to the fact that it is not the server checking if the port is reachable, but the yarp clean command. For example, in case of yarp server started with --ros, if rosmaster is not running, this will delete the /ros port, and therefore a restart of the yarp server will be required.
4. In some cases, the 3 second timeout might not be enough to ensure that the port is actually closed.

The proposal by @S-Dafarra is reasonable, and will fix the delay in case of a restart, but it is still problematic for non-YARP ports.
An option would be add an option when registering a non-YARP port (or when registering a YARP port), in order to know if the port should be deleted by YARP clean.

There are anyway several other safety related issues, for example, anyone can just call yarp name unregister /root or NetworkBase::unregisterName("/root") from code, and take possession of the name "/root" and much more.