Pass callable object by value, assign it to pointer member

Question

We received code from a subcontractor that does essentially the following:

class Callable
{
public:
    void operator()(int x)
    {
        printf("x = %d\n", x);
    }
};

template<typename T>
class UsesTheCallable
{
public:
    UsesTheCallable(T callable) :
            m_callable(NULL)
    {
        m_callable = &callable;
    }

    ~UsesTheCallable() {}

    void call() { (*m_callable)(5); }

private:
    T* m_callable;
};

This strikes me as being undefined code...they pass in the T by value to the UsesTheCallable constructor then assign the m_callable member to the address of the argument, which should go out of scope at tne end of the constructor, and so anytime I call UsesTheCallable::call(), I'm acting on an object that no longer exists.

So I tried it with this main method:

int main(int, char**)
{
    UsesTheCallable<Callable>* u = NULL;

    {
        Callable c;
        u = new UsesTheCallable<Callable>(c);
    }

    u->call();

    delete u;

    return 0;
}

I make sure that the Callable object goes out of scope before I call UsesTheCallable::call(), so I should be calling the function on memory that I don't actually own at that point. But the code works, and Valgrind reports no memory errors, even if I put some member data into the Callable class and make the operator() function act on that member data.

Am I correct that this code is undefined behavior? Is there any difference in the "defined-ness" of this code based on whether or not Callable has member data (e.g. a private int variable or something)?

Answer 1

Yes this is undefined behavior. After the closing brace of the constructor callable is destroyed and you have a dangling pointer.

The reason you are not seeing adverse effects is that you really aren't using the instance after it goes out of scope. The function call operator is stateless so it is not trying to access the memory that it no longer owns.

If we add some state to the callable like

class Callable
{
    int foo;
public:
    Callable (int foo = 20) : foo(foo) {}
    void operator()(int x)
    {
        printf("x = %d\n", x*foo);
    }
};

and then we use

int main()
{
    UsesTheCallable<Callable>* u = NULL;
    {
        Callable c(50);
        u = new UsesTheCallable<Callable>(c);
    }
    u->call();
    delete u;
    return 0;
}

Then you could see this bad behavior. In that run it outputs x = 772773112 which is not correct.

Answer 2

m_callable = &callable;

Am I correct that this code is undefined behavior?

Yes, this is bullsh!t, for the reason you give.

But the code works

Yeah, well, that's what happens with UB…

and Valgrind reports no memory errors

…particularly when the memory you're operating on still "belongs" to your process. There's nothing for Valgrind to detect here; it doesn't validate C++ scopes, only "physical"^† memory accesses. And the program doesn't crash because nothing's yet had much of a chance to mangle the memory that used to be taken up by c.

_{^† "Physical" in the sense that I'm referring to the OS and its memory management, rather than C++'s abstract concepts. It could actually be virtual memory or whatever.}