delete boost function while in use

Question

I have a situation where a boost::function and boost::bind (actually a std::tr1::function and bind) are being deleted while still in use. Is this safe? I would normally avoid it, but the offending code is a bit entrenched and my only other option is adding a new thread.

typedef function<int(int)> foo_type;

foo_type* global_foo = NULL;

int actual_foo( int i, Magic* m )
{
    delete global_foo;
    return m->magic(i);
}

int main()
{
  Magic m;
    global_foo = new foo_type( bind( &actual_foo, _1, &m );

    return (*global_foo)(10)
}

The bound parameters are always plain integral types (an int and a pointer in the real code), and not references.

Answer 1

Edit: Having discussed this answer a little more (with Charles Bailey), I believe this is unsafe since it depends on the implementation of boost::function.

The call stack when we enter actual_foo will look something like this:

actual_foo
boost::function::operator()
main

So when actual_foo has finished executing, we will jump back into the operator() code of boost::function, and this object has been deleted. This is not guaranteed to be a problem - it's a bit like calling delete this - but you have to be very careful what you do in a member function of a deleted object. You're not allowed to call any virtual functions or use any data members.

The problem is that I am not aware that boost::function makes any guarantees about what it does in operator() after the function it wraps has been called. It seems on my platform it doesn't do anything dangerous (so valgrind doesn't complain) but it is perfectly conceivable that on a different platform, with a different implementation, or with different compile flags, it might want to do something that isn't allowed once the object has been deleted - for example it might write out some debug information using a member variable.

Thus I believe this is a dangerous thing to do that might cause undefined behaviour under certain circumstances.

Further Note:

I took a quick look at what boost is really doing after it calls the function pointer. Looking here: http://boost.cvs.sourceforge.net/viewvc/boost/boost/boost/function/function_template.hpp?view=markup at the operator() function on line 687, my interpretation is that it immediately returns the return value and doesn't do anything else, so in practice, with that implementation, you should be ok, but the comments about it be dangerous still hold. Note that I may very well have found the wrong bit of code and/or understood it wrong...

Original answer below:

I believe this is ok. By the time you enter actual_foo, the boost::bind and boost::function objects have finished their jobs, and you are executing the real function actual_foo.

I checked this out on my platform (gcc 4.2.4, Linux) by running the program through valgrind.

Here is the program I ran:

#include <boost/bind.hpp>
#include <boost/function.hpp>

class Magic
{
public:
    int magic( int i )
    {
        return 5;
    }
};

typedef boost::function<int(int)> foo_type;

foo_type* global_foo = NULL;

int actual_foo( int i, Magic* m )
{
    delete global_foo;
    return m->magic(i);
}

int main()
{
    Magic m;
    global_foo = new foo_type( boost::bind( &actual_foo, _1, &m ) );

    return (*global_foo)(10);
}

and here is the valgrind output:

$ valgrind ./boost_bind
==17606== Memcheck, a memory error detector.
==17606== Copyright (C) 2002-2007, and GNU GPL'd, by Julian Seward et al.
==17606== Using LibVEX rev 1804, a library for dynamic binary translation.
==17606== Copyright (C) 2004-2007, and GNU GPL'd, by OpenWorks LLP.
==17606== Using valgrind-3.3.0-Debian, a dynamic binary instrumentation framework.
==17606== Copyright (C) 2000-2007, and GNU GPL'd, by Julian Seward et al.
==17606== For more details, rerun with: -v
==17606== 
==17606== 
==17606== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 17 from 1)
==17606== malloc/free: in use at exit: 0 bytes in 0 blocks.
==17606== malloc/free: 1 allocs, 1 frees, 16 bytes allocated.
==17606== For counts of detected errors, rerun with: -v
==17606== All heap blocks were freed -- no leaks are possible.

I must say, though, that this seems a strange thing to do. I would much prefer, if it were possible, to use automatic deletion of this function object by making it a stack variable, or deleting it in the destructor of a stack variable (as in RAII). It would be neater, safer, less scary, and more exception-safe. But I'm sure you already know all that, and there are good reasons for living with the code as it is.

Answer 2

boost::function or std::tr1::functions are copyable objects. So, generally there is absolutly no reason to allocate them -- just pass them by value.

They are well optimized for most of real cases... So just pass them by value:

typedef function<int(int)> foo_type;

foo_type global_foo;

int actual_foo( int i, Magic* m )
{
   delete global_foo;
   return m->magic(i);
}

int main()
{
   Magic m;
   global_foo = bind( &actual_foo, _1, &m );

   return global_foo(10)
}

The code you suggested is dangerous, run this code:

    #include <boost/function.hpp>
    #include <boost/bind.hpp>
    #include <iostream>
    using namespace std;

    boost::function<void()> *glb;

    struct Data {
            int x;
            Data(int _x = 0) : x(_x) { cout<<"ctor:"<<this<<endl; }
            ~Data() { cout<<"dtor:"<<this<<endl; }
            Data(Data const &p) {x=p.x; cout<<"ctor:"<<this<<endl; }
            Data const &operator=(Data const &p) { x=p.x; cout<<this<<"="<<&p<<endl; return *this; }
    };

    void func(Data const &x)
    {
            delete glb;
            cout<<&x<<endl;
    }

    int main()
    {
            glb=new boost::function<void()>(boost::bind(func,Data(3)));

            (*glb)();
            return 0;
    }

You would find that you try to access in func to destoryed object (dtor with same pointer value) that is shown in cout<<&x<<endl was already called.

Because when you destroy your function objects you also destroy the binded parameters you have and Data const &x become unavailible becuase it was destoryed with global_function

Edit: Clearification for comment:

If you have something like

map<string,function<void()> > calls;

void delete_key(){
    calls.erase("key");
}

main() 
{
     calls["key"]=delete_key;

     // Wrong and dangerous
     // You delete function while in use
     calls["key"]();

     // Correct and safe
     // You create a copy of function so it would not
     // be deleted while in use.
     function<void()> f=calls["key"];
     f();
}