Ensure compliant node construction/destruction API.

[hidmic]

Connected to ros2/rmw#249.

[ivanpauno]

nit: maybe defaulting to RMW_NODE_NAME_INVALID?
there's a unlikely chance of RMW_NODE_NAME_VALID being -1

[clalancette]

I took a quick look at the uses of rmw_validate_node_name throughout the code base. Currently, how we initialize validation_result is inconsistent. Some places, we don't initialize it at all, and the caller assumes rmw_validate_node_name fills it in with something valid. Some places, we initialize it to 0. And it also looks like the values that it can possibly return are just #defined: https://github.com/ros2/rmw/blob/ab3df8095cedf2c809c631e5c3442f964404e797/rmw/include/rmw/validate_node_name.h#L26 .

What I'll suggest here is:

1. Switch this to RMW_NODE_NAME_VALID (since there is no generic RMW_NODE_NAME_INVALID)
2. (separate PR) Change the definition of these values to an enum in rmw/validate_node_name.h
3. (separate PR) Add an enum value of RMW_NODE_NAME_UNKNOWN.
4. (separate PR) Update all of the callers of this method to use that.

What do people think of that plan?

[ivanpauno]

(separate PR) Add an enum value of RMW_NODE_NAME_UNKNOWN.

This new value would be for initializing the enum, right?

What do people think of that plan?

👍

[clalancette]

This new value would be for initializing the enum, right?

Yeah, exactly. It would ensure we never end up in a UB situation.

[hidmic]

Ok, I'll start out with option (1.) and then proceed with the rest in follow-up PRs.

[ivanpauno]

👍 for using the allocator in context->options, but is this change related?
what was the idea of the original rmw_node_allocate()?

[hidmic]

Yes and no. We don't have to, but I think we should settle on ros2/rmw#249 (comment).

[clalancette]

I see what you mean about this not using the allocator properly. But I think that is a separate topic, and so I would suggest leaving this as-is and we can discuss that separately.

[hidmic]

Fair enough.

[ivanpauno]

why not using the passed identifier argument?

[hidmic]

It's the same, though it seemed more correct to use the one in context. Actually, this function does not need to take a separate identifier but I chose not to change its signature.

[clalancette]

First pass, a number of things to think about.

[clalancette]

This assert seems a bit incongruous with the checking we do in e.g. rwm_create_node. That is, this crashes the program, while most other places we do checks like this we just return an error from the function. Also, this is somewhat of a philosophical debate, but my view is that a library should never assert; that's an application-level decision. I'll suggest something more like:

if (node->context != nullptr) {
  RCUTILS_SET_ERROR_MSG("Attempted to destroy invalid context");
  return RMW_RET_INVALID_ARGUMENT;
}

[hidmic]

This assert seems a bit incongruous with the checking we do in e.g. rmw_create_node.

How so? rmw_create_node() checks for a zero-initialized context because that's one of the possible rmw_context_t states (i.e. right before initialization and after finalization). There's no correct program that may call rmw_destroy_node() on a half-initialized or even uninitialized rmw_node_t structure.

Also, this is somewhat of a philosophical debate, but my view is that a library should never assert; that's an application-level decision.

I disagree. To return an error (or to throw an exception) implies there is a path to deal with the situation in a sane way. If you have hit UB territory, either your program is logically incorrect or it has managed to corrupt itself. There's no way to handle the situation other than to crash fast. That applies to applications and libraries the same.

[hidmic]

Which reminds me that we should document that after rmw_destroy_node(), using the pointer is UB.

[clalancette]

@hidmic and I discussed this at some length yesterday. The pretty short summary of the discussion goes something like this:

• The caller can do arbitrary things to make this UB. Like the following:

auto node = rmw_create_node(...);
memset(node, 0xFF, sizeof(rmw_node_t));
rmw_destroy_node(node);

There is nothing that we can do about cases like this; it's clearly an abuse of the API/library, and there is nothing we can reasonably check to defend against this.

• Accessing a nullptr here is really just another case of UB. Take the above example and use a 0x0 instead:

auto node = rmw_create_node(...);
memset(node, 0x0, sizeof(rmw_node_t));
rmw_destroy_node(node);

And you have the same problem. Thus, there's no real reason to treat nullptr specially here.

• The only other problem that we could potentially guard against is the user somehow intentionally setting the pointer to nullptr. We could have a check for that, but its pretty unlikely that the user has done this on purpose. Thus, for all practical purposes, this is UB too.

The conclusion from all of the above is that in all reasonable cases, it is UB, and thus we shouldn't have any checking here. That's why @hidmic removed both the assert and the check from this code.

[hidmic]

We could have a check for that, but its pretty unlikely that the user has done this on purpose.

For that matter, I'd say it's equally likely than reinterpret_cast<rmw_node_impl_t *>(0xFF). In both cases, the user is departing from what's documented and corrupting the structure with pointers that cannot be de-referenced.

[clalancette]

Same argument as above; this should check and return an error, I think.

[hidmic]

Same as above.

[clalancette]

I'd use the RMW_CHECK_ARGUMENT_FOR_NULL macros here.

[hidmic]

We could, though we'd be replicating the checks that occur in the actual RMW implementations. Since this is a somewhat "internal" API (not a standalone RMW implementation but the pieces of it), I figured we may as well keep these as preconditions instead of checking twice.

[clalancette]

Good point about it being an internal API. With that in mind, I think we can elide the checks completely. The basic idea here being that a library should be able to trust itself; anything else is just buggy. And as you say, these are duplicate checks anyway.

(the same goes for __rmw_destroy_node below)

[hidmic]

Agreed. I'd keep the implementation identifier assertion though, since it is a precondition we can fully enforce (as opposed to pointer validity) and failing to meet it implies an ill-formed program (that we want to catch as soon as possible if not preclude entirely).

[clalancette]

I took a quick look at the uses of rmw_validate_node_name throughout the code base. Currently, how we initialize validation_result is inconsistent. Some places, we don't initialize it at all, and the caller assumes rmw_validate_node_name fills it in with something valid. Some places, we initialize it to 0. And it also looks like the values that it can possibly return are just #defined: https://github.com/ros2/rmw/blob/ab3df8095cedf2c809c631e5c3442f964404e797/rmw/include/rmw/validate_node_name.h#L26 .

What I'll suggest here is:

1. Switch this to RMW_NODE_NAME_VALID (since there is no generic RMW_NODE_NAME_INVALID)
2. (separate PR) Change the definition of these values to an enum in rmw/validate_node_name.h
3. (separate PR) Add an enum value of RMW_NODE_NAME_UNKNOWN.
4. (separate PR) Update all of the callers of this method to use that.

What do people think of that plan?

[clalancette]

I see what you mean about this not using the allocator properly. But I think that is a separate topic, and so I would suggest leaving this as-is and we can discuss that separately.

[hidmic]

Running CI up to test_rmw_implementation and all RMW implementations:

• Linux
• Linux-aarch64
• macOS
• Windows

[clalancette]

I've got one more thing that I think we should change here. Other than that, this is looking good to me.

[clalancette]

Good point about it being an internal API. With that in mind, I think we can elide the checks completely. The basic idea here being that a library should be able to trust itself; anything else is just buggy. And as you say, these are duplicate checks anyway.

(the same goes for __rmw_destroy_node below)