Issues with function_ref

[ymarcov]

Required information

Operating system: Any

Compiler version: Any

Eclipse iceoryx version: master

Observed result or behaviour: In cxx::function_ref:

1. operator() is marked noexcept
2. Construction erroneously accepts rvalue capturing lambdas (i.e. [&] { ... })
3. Construction invokes UB when accepting rvalue non-capturing lambdas (i.e. [] { ... })

Expected result or behaviour:

1. operator() should not be marked noexcept, since it is unreasonable for the program to terminate if the user-code invoked throws an exception.
2. Capturing lambdas necessarily imply state, and it is a mistake that function_ref can be built with stateful rvalues. Attempting this construction should result in a compilation error.
3. Non-capturing lambdas should be okay for construction, since they are convertible to a function pointer. However, the current implementation still saves them as objects, and applies callable_object(args...) on them where callable_object is now necessarily a dangling reference.

[elfenpiff]

@ymarcov Thanks for the finding.

1. I think we can easily solve this by removing noexcept. Introducing noexcept was one of our early mistakes which we really have to face this year.

Could you elaborate 2 a bit more in detail. The contract is that a function_ref does not own the callable and the user has to ensure the lifetime of it by other means. If you require an owning function you have to use function. So a capturing lambda, or lambda with with state should be alright as long as it lifes at least as long as the function_ref. But I assume that you meaning constructs like (A):

function_ref f = [&]() { doStuff(capturedVariable); };
// the lambda is a temporary rvalue and the lifetime is shorter than f and we have a problem 

But in a construct like this it would be alright (B):

void callMe(const function_ref<void()> &f) {
  f();
} 

callMe([&]() { doStuff(capturedVariable);});

Here the C++ standard guarantees that the lambda is a temporary rvalue variable which lifes until the end of the statement and the call to the function is finished.

This would in summary mean that when we fix 2. we wouldn't be able to create a function_ref like in (A) which would prevent bugs but also our major use case (B) wouldn't work anymore and for iceoryx function_ref would maybe become useless.

If this is correct I would tend to remove function_ref completely since it is too easy to shoot yourself in the face and capturing lambdas is for me a must have otherwise I do not see any advantage over a simple and direct c function ref as argument.

[ymarcov]

@elfenpiff I think that it is indeed risky to be able to construct an invalid function_ref, especially in so easy a manner.

For your callMe() example, I'd probably replace that with

template <typename Func>
void callMe(Func&& f);

If the user didn't want to store this function-object beyond the call, it would still be legit to use like that. And if he did, then he'd have to store it in a normal function, which would be valid.

Re the removal of function_ref, hard for me to say since this is a useful construct, but if other than the use case you mentioned there is no real use for it in this codebase, then perhaps it could be removed.

[mossmaurice]

@ymarcov Thanks for creating this issue!

1. I agree with what was said above.
2. I understand the issue and that it can easily lead to misuse. However, my point is that with similar non-owning constructs the same issue exists (e.g. std::string_view, std::span, etc.). It was even considered in the proposal from 2019 in section 13. Using an address sanitizer to detect invalid memory access is mentioned there. The latest paper presents a survey on implementations in different base libraries. E.g. in Abseil a function_ref is not assignable to prevent subtle lifetime bugs, so for the iox::function_ref IMHO it should be removed as well. It might be worth looking at the other implementations in detail mentioned in the paper. I would refrain from removing the function_ref completely as non-owning wrapper like std::span and friends are very useful in embedded, real-time development. What we could discuss is to remove function_ref from the public API.
3. The solution could be to always use this c'tor for everything which is convertible to a function pointer. Need to think more about it, though.

cc @elfenpiff

[ymarcov]

@mossmaurice

1. Check.
2. Good argument IMO. One could accept a function_ref only when one is sure this won't be stored, otherwise (by default, maybe) accept a function.
3. Good solution IMO.

[elfenpiff]

@mossmaurice

But the abseil function ref has still this issue:

function_ref f{[&]() { doStuff(capturedVariable); }};
// dangling reference to temporary lambda

I think a function_ref can only be used safely when either capturing lambdas are forbidden or when we can forbid that the function_ref is stored anywhere. This can be partially ensured by converting the callable operator into an rvalue. Then a user could use it like this:

void callMe(function_ref<void()> &&f) {
  std::forward<function_ref<void()>>(f)(); //forward to keep the rvalue ness
} 

But it could also be misused like:

function_ref f{[&]() { doStuff(capturedVariable); }};
std::move(f)();

So the more I think about function_ref the more I am against using it. The reason is that one can misuse it so incredibly easy and that one captures something in the wrong context can be overlooked very easily.

Just think of the time when the function and function_ref was still nullable. We had a lot of bugs in our code were we just forgot to check if there is really a value contained and this despite we already have two reviewers per PR. This demonstrates to me, everything which the compiler or a test cannot directly and deterministically catch should be avoided. And the dangling reference issue of the funtion_ref is not going to be catched by the compiler. We weren't aware of the issue until @ymarcov discovered it and I would bet it would also slip future PR. And testing this issue is not going to work. It is undefined behavior anything can happen and in most cases it will work which is the most dangerous behavior.

With these arguments in mind I would remove function_ref.

@elBoberido what do you think?

[elBoberido]

@elfenpiff well, I was aware that storing a function_ref has issues and I always advocated to only use is as parameter in functions but never store it. We also mused a few times about the `_viewclasses of the STL, why I think they are a source for bugs and that they should only be used as "transport" type by not as "storage" type. Saying that,function_refhas it's use cases and as long as we can prevent a bug-prone usage like storing afunction_ref` I would keep it. Stuff like

function_ref f{[&]() { doStuff(capturedVariable); }};
std::move(f)();

I do not consider bug prone since it a deliberate decision of the user but allowing only rvalues would probably solve our issues.

Btw, it looks like we pass a reference of a function_ref as parameter at some places.

cc @ymarcov @mossmaurice

*) this does not mean that I do not make the mistake myself to accidentally recommend the use of a function_ref as template parameter which accepts any callable and stores that parameter like in the PeriodicTask 🙈

[ymarcov]

My feeling here is that it isn't possible to provide function_ref in a way that is both elegantly usable, as well as foolproof. Same as _view classes. But I'm okay with not making everything foolproof.

I would add here, though, that unlike string_view and friends which provide a non trivial and uniform API, I'm not sure that using function ref only for "transition" objects is better than a templated Callable. The latter is even more efficient as well as less bug prone since you can't easily store it without a safe wrapper such as storable function.

Except, it could be useful for virtual functions which cannot take a template, but from what I understand this is not a common use case for you anyway in iceoryx.

[ymarcov]

I liked the idea of @mossmaurice to allow this constructor, but provide a lint rule to check it is used correctly.

[elBoberido]

@ymarcov I'm passionless about keeping or removing function_ref. Using a template instead is not always the best solution since one suddenly has other issues like testing and ensuring to have the right restrictions, e.g. using is_invocable. From this point of view the function_ref has a better usability. On the other hand, nowadays we also have iox::function which also does not allocate but is a bit heavier than the function_ref. Do you think it would be possible to make function_ref foolproof and somewhat elegantly usable for the most use common use case as transition object and prevent the usage where this is not the case? The alternative would then be to use either templates or the iox::function.

cc @mossmaurice @elfenpiff

[mossmaurice]

@elfenpiff We weren't aware of the issue until @ymarcov discovered it and I would bet it would also slip future PR.

@elBoberido well, I was aware* that storing a function_ref has issues and I always advocated to only use is as parameter in functions but never store it.

That's not correct, we were aware see the doxygen.

Back during the first implementation, I agreed and still agree with what is written in the proposal:

I strongly believe that accepting temporaries is a “necessary evil” for both function_ref and std::string_view, as it enables countless valid use cases. The problem of dangling references has been always present in the language - a more general solution like Herb Sutter and Neil Macintosh’s lifetime tracking18 would prevent mistakes without limiting the usefulness of view/reference classes.

IMHO we can't change the pitfalls of the core C++ language without switching to other more modern languages like Rust, which offer concepts like borrowing.

With these arguments in mind I would remove function_ref.

If we apply your point, this would also mean std::span and std::string_view and friends should be avoided. However, those actually lead to safer code to prevent API which can lead to buffer overflows or out-out-bounds access like

copyBuffer(char* buffer, size_t size)

with

copyBuffer(std::span buffer)

but of course they can also be misused when using them not as function arguments, but to stored something.

My feeling here is that it isn't possible to provide function_ref in a way that is both elegantly usable, as well as foolproof.

Full ack :100:

provide a lint rule to check it is used correctly.

I like the idea! What I would propose is to write a custom-clang tidy rule which checks that those constructs are only used as function parameter. This could be even accepted upstream for string_view and friends. Maybe it already exists. Such a rule, good documentation and an address sanitizer to detect invalid memory access should provide enough safety to use them. Does that make sense?

cc @ymarcov