Fix warning: possible null dereference

[roncapat]

Closes #2493 CC @clalancette

At least for now I drafted a bad_alloc exception, but in the same files it is used std::runtime_error carrying always an explanatory message. bad_alloc by design is not allowed to allocate memory, so it is not designed with error messages in mind.

So I open this as draft so we can devise the best strategy / the one more consistent with rclcpp development in general.

[jmachowinski]

Can we move the check into the allocate functions ?

[alsora]

Can we move the check into the allocate functions ?

+1 on @jmachowinski suggestion

I think it's a good change and it could be worth to apply it automatically.

@roncapat note that the Jazzy feature freeze is on Monday. I think is worth to push this even if it's a API break, and we can document it in the changelog.

[alsora]

• Linux
• Linux-aarch64
• Windows

[roncapat]

I don't understand these CI failures. Anyway, sorry for being late on this, I'll try today to do an alternative PR with fixes inside the executor as suggested.

[roncapat]

I just gave a look to the code, and frankly speaking I don't understand whether there is a possibility to move the check in the allocator. As the default allocator is std::allocator<void>, we can't make it throw inside on ourselves (it's a standard library thing). Aside from all the templating magic there, what seems to happen under the hood is a call to std::allocator_traits<Alloc>::allocate passing the allocator, which by default is std::allocator<void>.

[roncapat]

Please also note that the current patch fixes unhandled nullptrs only in some rclcpp headers, the ones used by my components per #2493. If there are other ::allocate calls around rclcpp codebase, I believe these would be detected only by a full -fanalyzer rebuild of rclcpp package. So this fixes -fanalyzer builds of end users consuming rclcpp API, maybe not rclcpp codebase comprehensively.

[wjwwood]

std::allocator<void>::allocate() already throws std::bad_alloc: https://en.cppreference.com/w/cpp/memory/allocator/allocate

As to the CI failures that looks bizarre to me.

[wjwwood]

I assume the entry from https://gist.githubusercontent.com/alsora/a25f5d4cc933eb86df82f51cc0a9d09b/raw/6579317f4545b42dd007b95e750d1850090af4ec/rclcpp-2496.repos:

  ros2/rclcpp:
    type: git
    url: https://github.com/roncapat/rclcpp.git
    version: fix/fanalizer_possible_dereference

Points to an out-of-date branch? Maybe it needs to be merged with rolling first? When you provide a custom repos file it doesn't merge with rolling first (afaik, but I could be wrong about that).

[roncapat]

Yes, at least the documentation says so... we may be in front of a GCC bug then? If I'm using standard allocator I shouldn't see this warning popping up.

[roncapat]

OK, my fault... C++ is not officially supported yet. I leave the bug tracker here because I think it is interesting to have a pointer to current support status and may be very useful in the future.

Closing for now.

[wjwwood]

Could be, I'm not familiar with how a function can let the compiler know what it will do. I assume there is an attribute of some sort, maybe that's just missing. I think there's a version of new that doesn't throw, and instead returns nullptr, but I think you have to pass nothrow to get that. I don't see something equivalent for allocate().

Yeah here: https://en.cppreference.com/w/cpp/memory/new/operator_new#Version_5