lola: Initialize method InArgs and Return values on creation

Previously, we were creating the type erased buffer for the InArgs and
Return values and then reinterpreting the buffer as InArgs / Return
values. This is UB and also means that non-trivially default
constructible types were not properly initialized. We now explicitly
call the constructors of each of these args.

Author: bemerybmw

score/mw/com/impl/methods/proxy_method.h

@@ -146,6 +146,52 @@ score::Result<std::tuple<impl::MethodInArgPtr<ArgTypes>...>> AllocateImpl(
         std::move(method_in_arg_ptr_tuple));
 }
 
+/// \brief Initializes all InArgs by calling the default constructor for each argument.
+///
+/// This step is important to avoid undefined behaviour (interpreting uninitialized memory) and also to ensure that any
+/// non-trivially constructible types are properly initialized.
+template <typename... ArgTypes>
+ResultBlank InitializeInArgs(ProxyMethodBinding& binding, const std::size_t queue_size)
+{
+    for (std::size_t queue_index = 0U; queue_index < queue_size; ++queue_index)
+    {
+        auto allocated_in_args_storage = binding.GetInArgsBuffer(queue_index);
+        if (!allocated_in_args_storage.has_value())
+        {
+            return Unexpected(allocated_in_args_storage.error());
+        }
+        const auto deserialized_arg_pointers = impl::Deserialize<ArgTypes...>(allocated_in_args_storage.value());
+
+        // std::apply takes a callable and a tuple. It calls the callable with the arguments from the unpacked tuple.
+        // E.g. In this case, it will call the lambda, fn, with: `fn(get<0>(args), get<1>(args), ..., get<n>(args))`
+        std::apply(
+            [](typename std::add_pointer<ArgTypes>::type... arg_pointers) {
+                ((score::cpp::ignore = new (arg_pointers) ArgTypes{}), ...);
+            },
+            deserialized_arg_pointers);
+    }
+    return {};
+}
+
+/// \brief Initializes all Return values by calling the default constructor for each argument.
+///
+/// This step is important to avoid undefined behaviour (interpreting uninitialized memory) and also to ensure that any
+/// non-trivially constructible types are properly initialized.
+template <typename ReturnType>
+ResultBlank InitializeReturnValue(ProxyMethodBinding& binding, const std::size_t queue_size)
+{
+    for (std::size_t queue_index = 0U; queue_index < queue_size; ++queue_index)
+    {
+        auto allocated_return_value_storage = binding.GetReturnValueBuffer(queue_index);
+        if (!allocated_return_value_storage.has_value())
+        {
+            return Unexpected(allocated_return_value_storage.error());
+        }
+        score::cpp::ignore = new (allocated_return_value_storage->data()) ReturnType{};
+    }
+    return {};
+}
+
 /// \brief Checks, that all MethodInArgPtr arguments have the same queue_position_ and returns this common value.
 /// \details Will assert/terminate, if the queue_position_ values differ.
 /// \tparam MethodInArgPtrs Variadic template parameter pack for MethodInArgPtr types.

score/mw/com/impl/methods/proxy_method_base.h

@@ -53,6 +53,17 @@ class ProxyMethodBase
         proxy_base_ = proxy_base;
     }
 
+    /// \brief Default initializes each method InArg and Return value (if they exist)
+    ///
+    /// This function is called on creation of a Proxy during ProxyBase::SetupMethods. Since the binding creates a type
+    /// erased buffer in which the InArgs and Return value are created, each value must be explicitly instantiated to
+    /// begin the object lifetime and also perform the correct initialization (in case the type cannot be trivially
+    /// default constructed). We do this once on startup instead of in a call to Allocate() to prevent the type being
+    /// reinitialized on every method call. This potentially would have performance benefits but more importantly this
+    /// allows us to support "semi-dynamic" types in which a type dynamically allocates once on construction and the
+    /// constructor is then never called again.
+    virtual ResultBlank InitializeInArgsAndReturnValues() = 0;
+
   protected:
     /// \brief Size of the call-queue is currently fixed to 1! As soon as we are going to support larger call-queues,
     /// the call-queue-size shall be taken from configuration and handed over to ProxyMethod ctor.

score/mw/com/impl/methods/proxy_method_test.cpp

@@ -57,6 +57,15 @@ class TestProxyBase : public ProxyBase
     }
 };
 
+struct NonTriviallyConstructibleType
+{
+    NonTriviallyConstructibleType() : value{kInitialValue} {}
+
+    static constexpr std::int32_t kInitialValue{21};
+
+    std::int32_t value;
+};
+
 template <typename MethodType>
 class ProxyMethodTestFixture : public ::testing::Test
 {
@@ -115,6 +124,11 @@ using WithInArgs = ::testing::Types<InArgsAndReturn, InArgsOnly>;
 using WithoutInArgs = ::testing::Types<ReturnOnly, NoInArgsOrReturn>;
 using WithResult = ::testing::Types<InArgsAndReturn, ReturnOnly>;
 
+using NonTrivialConstructibleInArgsAndReturn = NonTriviallyConstructibleType(NonTriviallyConstructibleType,
+                                                                             NonTriviallyConstructibleType);
+using NonTrivialConstructibleInArgsOnly = void(NonTriviallyConstructibleType, NonTriviallyConstructibleType);
+using NonTrivialConstructibleReturnOnly = NonTriviallyConstructibleType();
+
 template <typename T>
 using ProxyMethodAllArgCombinationsTestFixture = ProxyMethodTestFixture<T>;
 TYPED_TEST_SUITE(ProxyMethodAllArgCombinationsTestFixture, AllArgCombinations, );
@@ -127,15 +141,18 @@ template <typename T>
 using ProxyMethodWithoutInArgsTestFixture = ProxyMethodTestFixture<T>;
 TYPED_TEST_SUITE(ProxyMethodWithoutInArgsTestFixture, WithoutInArgs, );
 
-template <typename T>
-using ProxyMethodWithResultTestFixture = ProxyMethodTestFixture<T>;
-TYPED_TEST_SUITE(ProxyMethodWithResultTestFixture, WithResult, );
-
 using ProxyMethodWithInArgsAndReturnFixture = ProxyMethodTestFixture<InArgsAndReturn>;
 using ProxyMethodWithReturnOnlyFixture = ProxyMethodTestFixture<ReturnOnly>;
 using ProxyMethodWithNoInArgsOrReturnFixture = ProxyMethodTestFixture<NoInArgsOrReturn>;
 using ProxyMethodWithInArgsOnlyFixture = ProxyMethodTestFixture<InArgsOnly>;
 
+using ProxyMethodWithNonTrivialConstructibleInArgsAndReturnFixture =
+    ProxyMethodTestFixture<NonTrivialConstructibleInArgsAndReturn>;
+using ProxyMethodWithNonTrivialConstructibleReturnOnlyFixture =
+    ProxyMethodTestFixture<NonTrivialConstructibleReturnOnly>;
+using ProxyMethodWithNonTrivialConstructibleInArgsOnlyFixture =
+    ProxyMethodTestFixture<NonTrivialConstructibleInArgsOnly>;
+
 TYPED_TEST(ProxyMethodAllArgCombinationsTestFixture, Construction)
 {
     // When constructing a ProxyMethod with all combinations of InArgs / return types
@@ -804,5 +821,174 @@ TEST(DetermineNextAvailableQueueSlot, DetermineNextAvailableQueueSlotCanFail)
     EXPECT_EQ(result, MakeUnexpected(ComErrc::kCallQueueFull));
 }
 
+TEST_F(ProxyMethodWithNonTrivialConstructibleInArgsAndReturnFixture, InitializeInArgsAndReturnValuesInitializesInArgs)
+{
+    this->GivenAValidProxyMethod();
+
+    // When calling InitializeInArgsAndReturnValues
+    const auto result = this->unit_->InitializeInArgsAndReturnValues();
+
+    // Then a valid result is returned
+    EXPECT_TRUE(result.has_value());
+
+    // and Allocate returns a pointer pointing to an initialized object (i.e. the non-trivial default constructor was
+    // called, initializing value to NonTriviallyConstructibleType::kInitialValue
+    auto method_in_arg_ptr_tuple = this->unit_->Allocate();
+    ASSERT_TRUE(method_in_arg_ptr_tuple.has_value());
+
+    auto& method_in_arg_ptr_0 = std::get<0>(method_in_arg_ptr_tuple.value());
+    NonTriviallyConstructibleType& in_arg_0{*method_in_arg_ptr_0};
+    EXPECT_EQ(in_arg_0.value, NonTriviallyConstructibleType::kInitialValue);
+
+    auto& method_in_arg_ptr_1 = std::get<1>(method_in_arg_ptr_tuple.value());
+    NonTriviallyConstructibleType& in_arg_1{*method_in_arg_ptr_1};
+    EXPECT_EQ(in_arg_1.value, NonTriviallyConstructibleType::kInitialValue);
+}
+
+TEST_F(ProxyMethodWithNonTrivialConstructibleInArgsAndReturnFixture,
+       InitializeInArgsAndReturnValuesPropagatesErrorFromGetInArgsBuffer)
+{
+    this->GivenAValidProxyMethod();
+
+    // Expect that GetInArgsBuffer is called once on the binding mock and returns an error.
+    EXPECT_CALL(this->proxy_method_binding_mock_, GetInArgsBuffer(0U))
+        .WillOnce(Return(MakeUnexpected(ComErrc::kBindingFailure)));
+
+    // When calling InitializeInArgsAndReturnValues
+    const auto result = this->unit_->InitializeInArgsAndReturnValues();
+
+    // Then an error is returned
+    ASSERT_FALSE(result.has_value());
+    EXPECT_EQ(result.error(), ComErrc::kBindingFailure);
+}
+
+TEST_F(ProxyMethodWithNonTrivialConstructibleInArgsAndReturnFixture,
+       InitializeInArgsAndReturnValuesPropagatesErrorFromGetReturnValueBuffer)
+{
+    this->GivenAValidProxyMethod();
+
+    // Expect that GetReturnValueBuffer is called once on the binding mock and returns an error.
+    EXPECT_CALL(this->proxy_method_binding_mock_, GetReturnValueBuffer(0U))
+        .WillOnce(Return(MakeUnexpected(ComErrc::kBindingFailure)));
+
+    // When calling InitializeInArgsAndReturnValues
+    const auto result = this->unit_->InitializeInArgsAndReturnValues();
+
+    // Then an error is returned
+    ASSERT_FALSE(result.has_value());
+    EXPECT_EQ(result.error(), ComErrc::kBindingFailure);
+}
+
+TEST_F(ProxyMethodWithNonTrivialConstructibleInArgsAndReturnFixture,
+       CallOperator_ZeroCopy_InitializeInArgsAndReturnValuesInitializesInReturn)
+{
+    this->GivenAValidProxyMethod();
+
+    // When calling InitializeInArgsAndReturnValues
+    this->unit_->InitializeInArgsAndReturnValues();
+
+    // Then the zero copy call operator returns a pointer pointing to an initialized object (i.e. the non-trivial
+    // default constructor was called, initializing value to NonTriviallyConstructibleType::kInitialValue
+    auto method_in_arg_ptr_tuple = this->unit_->Allocate();
+    ASSERT_TRUE(method_in_arg_ptr_tuple.has_value());
+
+    auto& method_in_arg_ptr_0 = std::get<0>(method_in_arg_ptr_tuple.value());
+    auto& method_in_arg_ptr_1 = std::get<1>(method_in_arg_ptr_tuple.value());
+    auto method_return_ptr = this->unit_->operator()(std::move(method_in_arg_ptr_0), std::move(method_in_arg_ptr_1));
+    ASSERT_TRUE(method_return_ptr.has_value());
+
+    NonTriviallyConstructibleType& return_value{*(method_return_ptr.value())};
+    EXPECT_EQ(return_value.value, NonTriviallyConstructibleType::kInitialValue);
+}
+
+TEST_F(ProxyMethodWithNonTrivialConstructibleInArgsAndReturnFixture,
+       CallOperator_WithCopy_InitializeInArgsAndReturnValuesInitializesInReturn)
+{
+    this->GivenAValidProxyMethod();
+
+    // When calling InitializeInArgsAndReturnValues
+    this->unit_->InitializeInArgsAndReturnValues();
+
+    // Then the copy call operator returns a pointer pointing to an initialized object (i.e. the non-trivial
+    // default constructor was called, initializing value to NonTriviallyConstructibleType::kInitialValue
+    auto method_return_ptr = this->unit_->operator()(NonTriviallyConstructibleType{}, NonTriviallyConstructibleType{});
+    ASSERT_TRUE(method_return_ptr.has_value());
+
+    NonTriviallyConstructibleType& return_value{*(method_return_ptr.value())};
+    EXPECT_EQ(return_value.value, NonTriviallyConstructibleType::kInitialValue);
+}
+
+TEST_F(ProxyMethodWithNonTrivialConstructibleInArgsOnlyFixture, InitializeInArgsAndReturnValuesInitializesInArgs)
+{
+    this->GivenAValidProxyMethod();
+
+    // When calling InitializeInArgsAndReturnValues
+    this->unit_->InitializeInArgsAndReturnValues();
+
+    // Then Allocate returns a pointer pointing to an initialized object (i.e. the non-trivial default constructor was
+    // called, initializing value to NonTriviallyConstructibleType::kInitialValue
+    auto method_in_arg_ptr_tuple = this->unit_->Allocate();
+    ASSERT_TRUE(method_in_arg_ptr_tuple.has_value());
+
+    auto& method_in_arg_ptr_0 = std::get<0>(method_in_arg_ptr_tuple.value());
+    NonTriviallyConstructibleType& in_arg_0{*method_in_arg_ptr_0};
+    EXPECT_EQ(in_arg_0.value, NonTriviallyConstructibleType::kInitialValue);
+
+    auto& method_in_arg_ptr_1 = std::get<1>(method_in_arg_ptr_tuple.value());
+    NonTriviallyConstructibleType& in_arg_1{*method_in_arg_ptr_1};
+    EXPECT_EQ(in_arg_1.value, NonTriviallyConstructibleType::kInitialValue);
+}
+
+TEST_F(ProxyMethodWithNonTrivialConstructibleInArgsOnlyFixture,
+       InitializeInArgsAndReturnValuesPropagatesErrorFromBinding)
+{
+    this->GivenAValidProxyMethod();
+
+    // Expect that GetInArgsBuffer is called once on the binding mock and returns an error.
+    EXPECT_CALL(this->proxy_method_binding_mock_, GetInArgsBuffer(0U))
+        .WillOnce(Return(MakeUnexpected(ComErrc::kBindingFailure)));
+
+    // When calling InitializeInArgsAndReturnValues
+    const auto result = this->unit_->InitializeInArgsAndReturnValues();
+
+    // Then an error is returned
+    ASSERT_FALSE(result.has_value());
+    EXPECT_EQ(result.error(), ComErrc::kBindingFailure);
+}
+
+TEST_F(ProxyMethodWithNonTrivialConstructibleReturnOnlyFixture,
+       CallOperator_WithCopy_InitializeInArgsAndReturnValuesInitializesInReturn)
+{
+    this->GivenAValidProxyMethod();
+
+    // When calling InitializeInArgsAndReturnValues
+    this->unit_->InitializeInArgsAndReturnValues();
+
+    // Then the copy call operator returns a pointer pointing to an initialized object (i.e. the non-trivial
+    // default constructor was called, initializing value to NonTriviallyConstructibleType::kInitialValue
+    auto method_return_ptr = this->unit_->operator()();
+    ASSERT_TRUE(method_return_ptr.has_value());
+
+    NonTriviallyConstructibleType& return_value{*(method_return_ptr.value())};
+    EXPECT_EQ(return_value.value, NonTriviallyConstructibleType::kInitialValue);
+}
+
+TEST_F(ProxyMethodWithNonTrivialConstructibleReturnOnlyFixture,
+       InitializeInArgsAndReturnValuesPropagatesErrorFromBinding)
+{
+    this->GivenAValidProxyMethod();
+
+    // Expect that GetReturnValueBuffer is called once on the binding mock and returns an error.
+    EXPECT_CALL(this->proxy_method_binding_mock_, GetReturnValueBuffer(0U))
+        .WillOnce(Return(MakeUnexpected(ComErrc::kBindingFailure)));
+
+    // When calling InitializeInArgsAndReturnValues
+    const auto result = this->unit_->InitializeInArgsAndReturnValues();
+
+    // Then an error is returned
+    ASSERT_FALSE(result.has_value());
+    EXPECT_EQ(result.error(), ComErrc::kBindingFailure);
+}
+
 }  // namespace
 }  // namespace score::mw::com::impl

score/mw/com/impl/methods/proxy_method_with_in_args.h

@@ -83,6 +83,8 @@ class ProxyMethod<void(ArgTypes...)> final : public ProxyMethodBase
     ProxyMethod(ProxyMethod&&) noexcept;
     ProxyMethod& operator=(ProxyMethod&&) noexcept;
 
+    ResultBlank InitializeInArgsAndReturnValues() override;
+
     /// \brief Allocates the necessary storage for the argument values and the return value of a method call.
     /// \return On success, a tuple of MethodInArgPtr for each argument type is returned. On failure, an error code is
     /// returned.
@@ -177,6 +179,18 @@ score::ResultBlank ProxyMethod<void(ArgTypes...)>::operator()(MethodInArgPtr<Arg
     return {};
 }
 
+template <typename... ArgTypes>
+ResultBlank ProxyMethod<void(ArgTypes...)>::InitializeInArgsAndReturnValues()
+{
+    SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD(binding_ != nullptr);
+    const auto init_in_args_result = detail::InitializeInArgs<ArgTypes...>(*binding_, kCallQueueSize);
+    if (!init_in_args_result.has_value())
+    {
+        return Unexpected(init_in_args_result.error());
+    }
+    return {};
+}
+
 }  // namespace score::mw::com::impl
 
 #endif  // SCORE_MW_COM_IMPL_METHODS_PROXY_METHOD_WITH_IN_ARGS_H

score/mw/com/impl/methods/proxy_method_with_in_args_and_return.h

@@ -95,6 +95,8 @@ class ProxyMethod<ReturnType(ArgTypes...)> final : public ProxyMethodBase
     ProxyMethod(ProxyMethod&&) noexcept;
     ProxyMethod& operator=(ProxyMethod&&) noexcept;
 
+    ResultBlank InitializeInArgsAndReturnValues() override;
+
     /// \brief Allocates the necessary storage for the argument values and the return value of a method call.
     /// \return On success, a tuple of MethodInArgPtr for each argument type is returned. On failure, an error code is
     /// returned.
@@ -214,6 +216,24 @@ score::Result<MethodReturnTypePtr<ReturnType>> ProxyMethod<ReturnType(ArgTypes..
         queue_position};
 }
 
+template <typename ReturnType, typename... ArgTypes>
+ResultBlank ProxyMethod<ReturnType(ArgTypes...)>::InitializeInArgsAndReturnValues()
+{
+    SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD(binding_ != nullptr);
+    const auto init_in_args_result = detail::InitializeInArgs<ArgTypes...>(*binding_, kCallQueueSize);
+    if (!init_in_args_result.has_value())
+    {
+        return Unexpected(init_in_args_result.error());
+    }
+
+    const auto init_return_result = detail::InitializeReturnValue<ReturnType>(*binding_, kCallQueueSize);
+    if (!init_return_result.has_value())
+    {
+        return Unexpected(init_return_result.error());
+    }
+    return {};
+}
+
 }  // namespace score::mw::com::impl
 
 #endif  // SCORE_MW_COM_IMPL_METHODS_PROXY_METHOD_WITH_IN_ARGS_AND_RETURN_H

score/mw/com/impl/methods/proxy_method_with_return_type.h

@@ -87,6 +87,8 @@ class ProxyMethod<ReturnType()> final : public ProxyMethodBase
     ProxyMethod(ProxyMethod&&) noexcept;
     ProxyMethod& operator=(ProxyMethod&&) noexcept;
 
+    ResultBlank InitializeInArgsAndReturnValues() override;
+
     /// \brief This is the call-operator of ProxyMethod with no arguments for a non-void ReturnType.
     score::Result<MethodReturnTypePtr<ReturnType>> operator()();
 
@@ -163,6 +165,18 @@ score::Result<MethodReturnTypePtr<ReturnType>> ProxyMethod<ReturnType()>::operat
         queue_position};
 }
 
+template <typename ReturnType>
+ResultBlank ProxyMethod<ReturnType()>::InitializeInArgsAndReturnValues()
+{
+    SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD(binding_ != nullptr);
+    const auto init_return_result = detail::InitializeReturnValue<ReturnType>(*binding_, kCallQueueSize);
+    if (!init_return_result.has_value())
+    {
+        return Unexpected(init_return_result.error());
+    }
+    return {};
+}
+
 }  // namespace score::mw::com::impl
 
 #endif  // SCORE_MW_COM_IMPL_METHODS_PROXY_METHOD_WITH_RETURN_TYPE_H

score/mw/com/impl/methods/proxy_method_without_in_args_or_return.h

@@ -83,6 +83,11 @@ class ProxyMethod<void()> final : public ProxyMethodBase
     ProxyMethod(ProxyMethod&&) noexcept;
     ProxyMethod& operator=(ProxyMethod&&) noexcept;
 
+    ResultBlank InitializeInArgsAndReturnValues() override
+    {
+        return {};
+    }
+
     /// \brief This is the call-operator of ProxyMethod with no arguments and a void ReturnType.
     score::ResultBlank operator()();