Lifetime issues with function calls on temporaries (e.g. 'auto i = T().f();' when f returns a reference)

Hello,

I use ChaiScript to perform interactive computations and visualizations. The scripting language works great and perfectly meets my needs for this application, thank you so much for all your work :slight_smile:

I have a question about the creation of a variable from the result of a function call returning a (possibly const) reference.
For instance auto i = a.i();, where the method A::i() returns a const int& (see code sample at the end of the post).
It seems that the variable i is not a copy of the integer, but rather a reference, as a modification of a leads to the modification of i in this example :

auto a = A(42); // int 42 wrapped in class A
auto i = a.i(); // get a's internal value
a += 10;        // increment a's internal value
print (i);      // prints 52

There seems to be no difference in this case between auto i = a.i(); and auto& i = a.i();.

It is of course possible to force a copy with auto i = int(a.i());, or with the function clone.
The need for an explicit copy in the script can however be easily forgotten, which may lead to unexpected results, and even undefined behaviour.
Consider for instance these other examples:

auto i = A(42).i();  // 'i' is a reference to a temporary attribute
print (i);           // prints random number (temporary is destroyed at this point)
auto a = getAs()[0]; // getAs() returns a 'std::vector<A>' with at least one 'A' 
                     //  'a' is a reference to an element of the temporary vector
print (a);           // prints random number (temporary destroyed)

I considered forbidding the assignment of a reference input through auto in ChaiScript, with this modification in language/chaiscript_eval.hpp (method Assign_Decl_AST_Node::eval_internal):

- Boxed_Value bv(detail::clone_if_necessary(this->children[1]->eval(t_ss), m_loc, t_ss));
+ auto incoming = this->children[1]->eval(t_ss);
+ if (incoming.is_ref()) 
+     throw exception::eval_error("... message to inform user to either use 'auto i = A(...)' pattern or 'auto& i = ...'");
+ Boxed_Value bv(detail::clone_if_necessary(std::move (incoming), m_loc, t_ss));

This modification replaces the silent failures shown above with error messages sent to the user, and doesn’t seem to have any other side effects.

But to avoid these error messages, and force a copy even when the constructor is not explicitely called by the user, I instead tried to insert an automatic call to the copy constructor when the incoming result is a reference.
When a reference is prefered (for instance because the copy is expensive, or to modify or follow the state of the object), it is still possible to use the explicit auto& i = ...;.

The following patch seems to work as expected for direct initializations such as auto i = ...;:

- Boxed_Value bv(detail::clone_if_necessary(this->children[1]->eval(t_ss), m_loc, t_ss));
+ auto incoming = this->children[1]->eval(t_ss);
+ if (incoming.is_ref()) { // interleave copy construction if result is a reference
+     const auto typeName = t_ss->get_type_name(incoming.get_type_info());
+     std::array<Boxed_Value, 1> params{std::move(incoming)};
+     incoming = t_ss->call_function(typeName, m_loc, Function_Params{params}, t_ss.conversions());
+ }
+ Boxed_Value bv(detail::clone_if_necessary(std::move (incoming), m_loc, t_ss));

A similar modification in Equation_AST_Node::eval_internal, covers delayed initialisations as well (e.g. auto i; i = ...;):

+ if (params[1].is_ref()) { // interleave copy construction if result is a reference 
+ 	const auto typeName = t_ss->get_type_name(params[1].get_type_info());
+ 	std::array<Boxed_Value, 1> params2{std::move(params[1])};
+ 	params[1] = t_ss->call_function(typeName, m_clone_loc, Function_Params{params2}, t_ss.conversions());
+ }
= params[1] = detail::clone_if_necessary(std::move(params[1]), m_clone_loc, t_ss);

Note : I tried to change the condition in method ‘clone_if_necessary’, to force cloning when ‘incoming.is_ref()’ is true, but it did not cover the case ‘auto a = getAs()[0];’.

These 2 patches lead to the apparition of a new problem with delayed reference initializations (auto& ir; ir = ...;). Here ir no longer acts as a reference.
To counter that, I added the method void set_is_ref(bool is_ref) const noexcept { m_data->m_is_ref = is_ref; } in class Boxed_Value, and tested the following modifications to keep track of the reference tag on the target variable, and disable the copy when it is a reference:

  • in Equation_AST_Node::eval_internal (modification of the previous patch):
- if (params[1].is_ref()) { // interleave copy construction if result is a reference
+ if (params[1].is_ref() && !params[0].is_ref()) { // interleave copy construction if result is a reference and target is not
  • in Reference_AST_Node::eval_internal:
= Boxed_Value bv;
+ bv.set_is_ref(true); // tag target as reference
= t_ss.add_object(this->children[0]->text, bv);
  • in Global_Decl_AST_Node::eval_internal:
- return t_ss->add_global_no_throw(Boxed_Value(), idname);
+ Boxed_Value bv;
+ bv.set_is_ref(this->children[0]->identifier == AST_Node_Type::Reference); // tag target as reference according to the presence of '&'
+ return t_ss->add_global_no_throw(bv, idname);

So far, this too seems to work on my tests.

Do you think these modifications may have side effects, may fail, or may lead to another unexpected behaviour?
Do you see an alternative option to avoid the creation of these dangling references?
Is trying to force the copy (when auto = ...; is used) a good idea in the first place?

I am using ChaiScript develop branch. Please find below some samples of code and script used to illustrate the issue.

Looking forward to your answer :slight_smile:
Best regards,
Skril


test.cpp:

#include <chaiscript/chaiscript.hpp>
#include <chaiscript/dispatchkit/bootstrap_stl.hpp>
#include <memory>

struct A {
public:
    explicit A(int i = 42): i_{i} {}
    const int& i() const { return i_; }

    A& operator+=(int i) { i_ += i; return *this; }
private:
    int i_;
};

int main() {
    try {
		chaiscript::ChaiScript chai;

		auto module = std::make_shared<chaiscript::Module>();
		chaiscript::utility::add_class<A>(*module,
			"A",
			{
				chaiscript::constructor<A(const A&)>(),
				chaiscript::constructor<A(int)>(),
			},
			{
				{chaiscript::fun(static_cast<A& (A::*)(const A&)>(&A::operator=)), "="},
				{chaiscript::fun(&A::i), "i"},

				{chaiscript::fun([](A a){ return std::to_string(a.i()); }), "to_string"},

				{chaiscript::fun(&A::operator+=), "+="}
			}
		);
		chaiscript::bootstrap::standard_library::vector_type<std::vector<A>>("As", *module);
		chai.add(module);

		chai.eval_file("test.chai");
    }
    catch (std::exception& e) {
        std::cerr << "\n" << e.what() << "\n";
    }
}

test.chai:

auto a = A(42); // int 42 wrapped in class A
auto i = a.i(); // get a's internal value
a += 10;        // increment a's internal value
print (i);      // prints 52, 42 after patch

auto a1; // idem, delayed initializations
a1 = A(42);
auto i1;
i1 = a1.i();
a1 += 10;        
print (i1);     // prints 52, 42 after patch 

// here a.i == 52

// additional tests for references and globals
auto& ir = a.i(); 
auto& ir1; 		
global& igr = a.i(); 		
global& igr1; 		
ir1 = a.i(); 
igr1 = a.i(); 
global ig = a.i();
global ig1;
ig1 = a.i();

a += 5; // now a.i == 57, so are references, but after patch raw globals still contains a.i's old value (52)

print (ir);     // prints 57
print (ir1);    // prints 57 
print (igr);    // prints 57 
print (igr1);   // prints 57 
print (ig);     // prints 57, 52 after patch
print (ig1);    // prints 57, 52 after patch

// some functions:
// get a vector of 'A's
def getAs() {
	auto as = As();
	as.push_back(A(42));
	return as;
}
// get an 'A'
def getA() {
	return A(42);
}

// test dangling references
for (auto i = 0; i < 100; ++i) {
	auto i2 = a.i();		// OK: always displays 57 in print call below
	auto i3 = A(42).i();		// NOK before patch (random number), seems ok after patch
	auto a4 = getAs()[0];  		// idem i3
	auto i5;
	i5 = getAs()[0].i();  		// idem i3
	auto a_ = getAs()[0];  		//  (to augment the probability of failure for i5)
	auto i6 = getAs()[0].i();  	// idem i3
	auto a__ = getAs()[0];  	//  (to augment the probability of failure for i6)	
	auto i7 = getA().i();  		// idem i3
	print("${i2} ${i3} ${a4} ${i5} ${i6} ${i7}"); // prints 57 42 42 42 42 42 after patch
}

In addition, a similar problem of dangling reference happens during the inline initialization of Vectors and Maps (and with Vector.push_back, which calls push_back_ref internally when the incoming value is a return value). Consider for example:

for (auto i = 0; i < 50; ++i) {
	auto m = ["0": A(42).i()]; // as in the previous post, i() returns a 
	auto v = [A(42).i()];      //  const reference to an attribute of type int
	v.push_back(A(42).i());    //  of the object of type A
	print (m); // random value mapped to key "0"
	print (v); // 2 random values 
}

If the previous patches are correct, then the three following modifications may also work around these occurrences of the problem:

  • dispatchkit/bootstrap_stl.hpp, back_insertion_sequence_type, in the definition of the push_back method:
- "  if (x.is_var_return_value()) {\n"
+ "  if (x.is_var_return_value() && !x.is_var_reference()) {\n" // copy the element if the incoming boxed value is a reference
  • language/chaiscript_eval.hpp, Inline_Array_AST_Node::eval_internal :
- vec.push_back(detail::clone_if_necessary(child->eval(t_ss), m_loc, t_ss));
+ auto incoming = child->eval(t_ss);
+ if (incoming.is_ref()) { // interleave copy construction if result is a reference 
+     const auto typeName = t_ss->get_type_name(incoming.get_type_info());
+     std::array<Boxed_Value, 1> params{std::move(incoming)};
+     incoming = t_ss->call_function(typeName, m_loc, Function_Params{params}, t_ss.conversions());
+ }
+ vec.push_back(detail::clone_if_necessary(std::move (incoming), m_loc, t_ss));
  • language/chaiscript_eval.hpp, Inline_Map_AST_Node::eval_internal:
- retval.insert(std::make_pair(t_ss->boxed_cast<std::string>(child->children[0]->eval(t_ss)), 
-             detail::clone_if_necessary(child->children[1]->eval(t_ss), m_loc, t_ss)));
+ auto incoming = child->children[1]->eval(t_ss);
+ if (incoming.is_ref()) { // interleave copy construction if result is a reference 
+     const auto typeName = t_ss->get_type_name(incoming.get_type_info());
+     std::array<Boxed_Value, 1> params{std::move(incoming)};
+     incoming = t_ss->call_function(typeName, m_loc, Function_Params{params}, t_ss.conversions());
+ }
+ retval.insert(std::make_pair(t_ss->boxed_cast<std::string>(child->children[0]->eval(t_ss)), 
+               detail::clone_if_necessary(std::move(incoming), m_loc, t_ss)));

As the same transformation is finally applied at each call site of clone_if_necessary, the call to the copy constructor could eventually be moved into that function. An extra argument would be needed, to inhibit the copy when the target is itself a reference (cf. second modification of Equation_AST_Node::eval_internal in the previous post).

After this second patch, values present in the Map and Vector constructed in the above example seem to be the expected ones.


Edit: The constructor of Pair taking 2 values has the same issue:

auto i = 0;
auto& j = i;

auto p0 = Pair(getA().i(), getA().i()); // references to temporaries are assigned to p0
auto p1 = Pair(i, j); // references of i and j are assigned to p1
auto p2 = Pair();
p2.first  = i; // copies of i and j are assigned to p2
p2.second = j;

++i; // modify i

print (p0); // random
print (p1); // <1, 1>
print (p2); // <0, 0>

This might be a workaround:

  • dispatchkit/bootstrap_stl.hpp, function pair_type:
+ if (typeid(typename PairType::first_type) != typeid(Boxed_Value)) // second_type not checked to keep Map_Pair unchanged
=     m.add(constructor<PairType (const typename PairType::first_type &, const typename PairType::second_type &)>(), type);
  • language/chaiscript_prelude.hpp:
+def Pair(x, y) {
+  auto p = Pair();
+  p.first = x;
+  p.second = y;
+  return p;
+}

Edit 2: Sticky references are in fact propagated every time a Boxed_Value is copied.
Hence the issue also happens in the following cases for example:

  • resize on a Vector with a provided value,
  • copy and assignment of a Vector,
  • inside concat function,
  • copy construction of a Pair (missing assignment),
  • copy and assignment of Map (and maybe other operations on Map).

The changes below replace these shallow copies with deep copies of the boxed values (except for Maps), and seem to work around the issue for Vector and Pair.
To try to minimize the side effects in scripts, I kept and renamed Vector to ChaiVector, and added using Vector = boost::container::small_vector<Boxed_Value, 1>; (use of small_vector only to differentiate from std::vector<Boxed_Value>; maybe a simple tag would be enough) :

  • chaiscript/chaiscript_stdlib.hpp:
- bootstrap::standard_library::vector_type<std::vector<Boxed_Value> >("Vector", *lib);
+ bootstrap::standard_library::vector_type<std::vector<Boxed_Value> >("ChaiVector", *lib);
+ bootstrap::standard_library::vector_type<Vector>("Vector", *lib);
  • dispatchkit/bootstrap_stl.hpp:
= template<typename ContainerType>
= void assignable_type(const std::string &type, Module& m)
= {
+   if constexpr (std::is_same_v<ContainerType, Vector>) {
+     m.eval("def " + type + "::`=`(" + type + " rhs){ \n"
+            "  this.clear();\n"
+            "  this.reserve(rhs.size());\n"
+            "  for (val: rhs){\n"
+            "    this.push_back(clone(val));\n" // ensure deep copy
+            "  }\n"
+            "} \n"
+            "def " + type + " (" + type + " rhs){ \n"
+            "  auto v := " + type + "();\n"
+            "  v.reserve(rhs.size());\n"
+            "  for (val: rhs){\n"
+            "    v.push_back(clone(val));\n" // ensure deep copy
+            "  }\n"
+            "  v;\n"
+            "} \n");
+   }
+   else {
=     copy_constructor<ContainerType>(type, m);
=     operators::assign<ContainerType>(m);
+   }
= }
= template<typename ContainerType>
= void resizable_type(const std::string & type, Module& m)
= {
+   if constexpr (std::is_same_v<ContainerType, Vector>) {
+     m.eval("def resize(" + type + " container, n, val){ \n"
+            "  auto sz := container.size();\n"
+            "  if (n > sz) {\n"
+            "    container.reserve(n);\n"
+            "    for (auto i = sz; i < n; ++i){\n"
+            "      container.push_back(clone(val));\n" // ensure deep copy
+            "    }\n"
+            "  }\n"
+            "} \n");
+   }
+   else {
=     m.add(fun([](ContainerType *a, typename ContainerType::size_type n, const typename ContainerType::value_type& val) { return a->resize(n, val); } ), "resize");
+   }
=   m.add(fun([](ContainerType *a, typename ContainerType::size_type n) { return a->resize(n); } ), "resize");
= }
= template<typename VectorType>
= void vector_type(const std::string &type, Module& m)
= {
=   /* ... */
-   if (typeid(VectorType) == typeid(std::vector<Boxed_Value>))
+   if (typeid(typename VectorType::value_type) == typeid(Boxed_Value)) 			  
=   {
-     m.eval(R"(
-             def Vector::`==`(Vector rhs) {
+     m.eval("def " + type + "::`==`(" + type + R"( rhs) {
=             /* ... */
=             } )"
=            );
=   } 
= } 
= template<typename PairType>
= void pair_type(const std::string &type, Module& m)
= {
=   /* ... */
-   basic_constructors<PairType>(type, m);
+   if constexpr (std::is_same_v<PairType, std::pair<Boxed_Value, Boxed_Value>>) {
+     m.eval("def " + type + "::`=`(" + type + " rhs){ \n"
+            "  this.first = rhs.first;\n"
+            "  this.second = rhs.second;\n"
+            "} \n"
+            "def " + type + " (" + type + " rhs){ \n"
+            "  " + type + "(rhs.first, rhs.second);\n"
+            "} \n");
+     m.add(constructor<PairType ()>(), type);
+   }
+   else {
+     basic_constructors<PairType>(type, m);
+     if constexpr (!std::is_const_v<typename PairType::first_type> && !std::is_const_v<typename PairType::second_type>)
+       operators::assign<PairType>(m);
+     }
+   }
=   /* ... */
= } 
  • language/chaiscript_eval.hpp, Ranged_For_AST_Node::eval_internal:
+ if (range_expression_result.get_type_info().bare_equal_type_info(typeid(Vector))) 
+   return do_loop(boxed_cast<const Vector&>(range_expression_result));
+ else 
=   if (range_expression_result.get_type_info().bare_equal_type_info(typeid(std::vector<Boxed_Value>))) {
  • language/chaiscript_eval.hpp, Inline_Array_AST_Node::eval_internal:
- std::vector<Boxed_Value> vec;
+ Vector vec; 

Previous changes did not completely get rid of the lifetime issue. It only reduced the duration of the dangling references, and the probability of an error. Erroneous values continued to appear from time to time.

In particular, with the example auto res = A(42).i(), the Boxed_Value holding A(42) is destroyed immediately after the computation of this->children[1]->eval(t_ss); in Assign_Decl_AST_Node::eval_internal, leading to a dangling reference in bv, ultimately assigned to res.
A run with -fsanitize=address indeed detects the heap-use-after-free in this scenario.

I added the attribute std::vector<std::shared_ptr<Data>> m_temporaries; in Boxed_Value::Data, to extend the lifetime of temporaries when Boxed_Values hold references. This new vector is initialized in the constructor of Data with the associated dependencies, in the following cases:

  • Attribute_Access::do_call: bv (i.e. params[0]),
  • dispatch::detail::call_func: params,
  • Static_Caster::cast and Dynamic_Caster::cast: t_from,
  • Ranged_For_AST_Node::eval_internal::do_loop: range_expression_result.

The dependencies propagates through the handle method for:

  • Handle_Return<Ret *&>,
  • Handle_Return<const Ret *&>,
  • Handle_Return<Ret *>,
  • Handle_Return<const Ret *>,
  • Handle_Return_Ref,
  • Handle_Return<Ret &>.

This modification seems to eventually cope with the issue of lifetime extension of temporaries (no more heap-use-after-free) for my use cases. This enhanced safety however certainly comes with additional runtime cost.