villapx villapx - 4 months ago 14
C++ Question

Pass callable object by value, assign it to pointer member

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

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;
}

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