How to fix undefined behaviour with considerably small changes in code

user_1498580 0
Viewed 110

In the core of our project we have following code:

template<typename T>
T& get_factory_instance(bool reset = false)
{
   static boost::scoped_ptr<T> factory_instance;
   if (reset)
   {
      factory_instance.reset();
      return *(T*)0;
   }
   if (!factory_instance)
   {
      factory_instance.reset(new T());
   }
   return *factory_instance;
}

If reset is true, that it's UB according to standard.

This function with argument true is called only when return value is ignored, so, memory is not accessed. It's mandatory, function cannot be called from services, only from libraries, when we add call of this function to singleton list of functions with signature void(). We need such strange hack for clearing prepared statement connected to the database, if connection lost.

So, basically question is: can it fire, if we don't access this memory? And if yes, how can we possibly fix it without rewriting all code dependent on this function, if there is any possibility?

When T constructor is called it constructs prepared statement and open/use connection to the database, so, creating dummy object is not pretty idea.

We have near 80 calls of this function with argument true in out libraries. It was introduced in 2015. Code that uses this function without argument in most cases looks like:

fields_t& fields = get_factory_instance<fields_t>();

Thanks in advance.

undefined-behavior reference c++
1 Answers

I understand that this is legacy code and you want to have no changes on the calls if possible. Further I assume all calls to the function are either

fields_t& fields = get_factory_instance<fields_t>();

or

get_factory_instance<fields_t>(true);

In other words, you never call it via

fields_t& fields = get_factory_instance<fields_t>(false);

Though as you will see, this actually wouldn't be a big issue, because with the following solution it would result in a compiler error and can be fixed easily.

You can refactor to:

template<typename T>
boost::scoped_ptr<T>& get_impl(){
    static boost::scoped_ptr<T> instance;
    return instance;
}
// or store the instance elsewhere
// and then...

template <typename T>
void get_factory_instance(bool) {
    get_impl<T>().reset();
}

template<typename T>
T& get_factory_instance() {
   auto& factory_instance = get_impl<T>();
   if (!factory_instance)
   {
      factory_instance.reset(new T());
   }
   return *factory_instance;
}

Alternatively, do use some dummy default static T to which you can return a reference, though that would be rather wasteful.

In general, you cannot safely return a T& when you have no T to refer to, so making the "reset" calls call a void function is the only option i see.

Well ok, instead of adding a void overload you could return a proxy object that only converts to T& when requested by the caller. Take this with a grain of salt, it is definitely more error prone than the above. I am not really recommending it, it is merely to show another possibility:

#include <iostream>

template <typename T>
struct maybe_ref_from_ptr { // first attempt of naming was optional_... but thats too misleading
    T* ptr;
    operator T& () { return *ptr; }
};

template <typename T>
maybe_ref_from_ptr<T> foo(bool reset = false) {
    static T* p = nullptr;
    if (reset ) {
        if(p) delete p;
        p = nullptr;
    } else {
        if (p == nullptr) p = new T(42);
    }
    return {p};
}

int main(){
    int& i = foo<int>();
    std::cout << i;
    foo<int>(true);
    int& j = foo<int>();
    std::cout << j;
}

For the real case the proxy would have to be adjusted to not simple store a raw pointer to a T but appropriate smart pointer. The obvious downside is that now the responsibility to avoid the UB is on the caller. Though you could throw in operator T& when the pointer isnt valid... I admit I didn't think this through completely, but I think you get the idea.

user_41177280
Related