Repository: OpenAssetIO/OpenAssetIO (language: python)

Issue:
`TraitsData::make` copy-constructor factory segfaults if param is `nullptr`
## What

If `TraitsData::make` is given a `TraitsDataConstPtr` that holds a `nullptr`, then it will attempt to dereference it and segfault.

In particular, this affects `Manager.createChildContext` when the parent `Context` has no `locale`.

## ACs

- `TraitsData::make` throws `InputValidation` if the supplied ptr is `null`, binding for cc has `.none(false)` (predicates c++ tests)
- `Context` make initializes `locale` with an empty `TraitsData` is none is supplied

`TraitsData::make` copy-constructor factory segfaults if param is `nullptr`
## What

If `TraitsData::make` is given a `TraitsDataConstPtr` that holds a `nullptr`, then it will attempt to dereference it and segfault.

In particular, this affects `Manager.createChildContext` when the parent `Context` has no `locale`.

## ACs

- `TraitsData::make` throws `InputValidation` if the supplied ptr is `null`, binding for cc has `.none(false)` (predicates c++ tests)
- `Context` make initializes `locale` with an empty `TraitsData` is none is supplied



Discussion / hints:

Fix patch:
diff --git a/RELEASE_NOTES.md b/RELEASE_NOTES.md
index 11215ec4d..0892bd514 100644
--- a/RELEASE_NOTES.md
+++ b/RELEASE_NOTES.md
@@ -10,6 +10,16 @@ v1.0.0-beta.x.x
   in Python.
   [#1186](https://github.com/OpenAssetIO/OpenAssetIO/issues/1186)
 
+### Bug fixes
+
+- Throw an exception when attempting to copy-construct a null
+  `TraitsData`.
+  [#1153](https://github.com/OpenAssetIO/OpenAssetIO/issues/1153)
+
+- Modified no-argument constructor of `Context` to create an empty (but
+  non-null) `locale`.
+  [#1153](https://github.com/OpenAssetIO/OpenAssetIO/issues/1153)
+
 v1.0.0-beta.1.0
 ---------------
 
diff --git a/src/openassetio-core/include/openassetio/Context.hpp b/src/openassetio-core/include/openassetio/Context.hpp
index 8b4646851..2a546567e 100644
--- a/src/openassetio-core/include/openassetio/Context.hpp
+++ b/src/openassetio-core/include/openassetio/Context.hpp
@@ -1,14 +1,12 @@
 // SPDX-License-Identifier: Apache-2.0
 // Copyright 2013-2022 The Foundry Visionmongers Ltd
 #pragma once
-#include <array>
-#include <cstddef>
 #include <memory>
 
 #include <openassetio/export.h>
+#include <openassetio/trait/TraitsData.hpp>
 #include <openassetio/typedefs.hpp>
 
-OPENASSETIO_FWD_DECLARE(trait, TraitsData)
 OPENASSETIO_FWD_DECLARE(managerApi, ManagerStateBase)
 
 namespace openassetio {
@@ -73,7 +71,7 @@ class OPENASSETIO_CORE_EXPORT Context final {
    * @fqref{hostApi.Manager.createContext} "Manager.createContext"
    * should always be used instead.
    */
-  [[nodiscard]] static ContextPtr make(trait::TraitsDataPtr locale = nullptr,
+  [[nodiscard]] static ContextPtr make(trait::TraitsDataPtr locale = trait::TraitsData::make(),
                                        managerApi::ManagerStateBasePtr managerState = nullptr);
 
  private:
diff --git a/src/openassetio-core/src/trait/TraitsData.cpp b/src/openassetio-core/src/trait/TraitsData.cpp
index 94b64827c..527c31591 100644
--- a/src/openassetio-core/src/trait/TraitsData.cpp
+++ b/src/openassetio-core/src/trait/TraitsData.cpp
@@ -3,6 +3,7 @@
 
 #include <unordered_map>
 
+#include <openassetio/errors/exceptions.hpp>
 #include <openassetio/trait/TraitsData.hpp>
 
 namespace openassetio {
@@ -90,6 +91,9 @@ TraitsDataPtr TraitsData::make(const trait::TraitSet& traitSet) {
 }
 
 TraitsDataPtr TraitsData::make(const TraitsDataConstPtr& other) {
+  if (!other) {
+    throw errors::InputValidationException("Cannot copy-construct from a null TraitsData");
+  }
   return std::shared_ptr<TraitsData>(new TraitsData(*other));
 }
 
diff --git a/src/openassetio-python/cmodule/src/ContextBinding.cpp b/src/openassetio-python/cmodule/src/ContextBinding.cpp
index 2fc6b4011..99ec906ed 100644
--- a/src/openassetio-python/cmodule/src/ContextBinding.cpp
+++ b/src/openassetio-python/cmodule/src/ContextBinding.cpp
@@ -14,15 +14,26 @@ void registerContext(const py::module& mod) {
   using openassetio::Context;
   using openassetio::ContextPtr;
   using openassetio::managerApi::ManagerStateBasePtr;
-  using openassetio::trait::TraitsDataPtr;
+  using openassetio::trait::TraitsData;
+  using PyRetainingTraitsDataPtr =
+      openassetio::PyRetainingSharedPtr<openassetio::trait::TraitsData>;
   using PyRetainingManagerStateBasePtr =
       openassetio::PyRetainingSharedPtr<openassetio::managerApi::ManagerStateBase>;
 
   py::class_<Context, ContextPtr> context{mod, "Context", py::is_final()};
 
   context
-      .def(py::init(RetainCommonPyArgs::forFn<&Context::make>()),
-           py::arg_v("locale", TraitsDataPtr{}), py::arg_v("managerState", ManagerStateBasePtr{}))
+      .def(py::init(
+               [](PyRetainingTraitsDataPtr locale, PyRetainingManagerStateBasePtr managerState) {
+                 return Context::make(std::move(locale), std::move(managerState));
+               }),
+           py::arg("locale").none(false), py::arg("managerState") = ManagerStateBasePtr{})
+      // Allow no-argument construction. Must use a separate constructor
+      // overload, rather than use a default value for `locale` above,
+      // since we must create a new `TraitsData` rather than re-use the
+      // same default `TraitsData` instance, where any mutations will
+      // persist in the default.
+      .def(py::init([]() { return Context::make(TraitsData::make(), ManagerStateBasePtr{}); }))
       .def_readwrite("locale", &Context::locale)
       .def_property(
           "managerState", [](const Context& self) { return self.managerState; },
diff --git a/src/openassetio-python/cmodule/src/trait/TraitsDataBinding.cpp b/src/openassetio-python/cmodule/src/trait/TraitsDataBinding.cpp
index ccc9f16a7..141b5ff66 100644
--- a/src/openassetio-python/cmodule/src/trait/TraitsDataBinding.cpp
+++ b/src/openassetio-python/cmodule/src/trait/TraitsDataBinding.cpp
@@ -23,7 +23,7 @@ void registerTraitsData(const py::module& mod) {
       .def(py::init(static_cast<TraitsDataPtr (*)(const trait::TraitSet&)>(&TraitsData::make)),
            py::arg("traitSet"))
       .def(py::init(static_cast<TraitsDataPtr (*)(const TraitsDataConstPtr&)>(&TraitsData::make)),
-           py::arg("other"))
+           py::arg("other").none(false))
       .def("traitSet", &TraitsData::traitSet)
       .def("hasTrait", &TraitsData::hasTrait, py::arg("traitId"))
       .def("addTrait", &TraitsData::addTrait, py::arg("traitId"))


Test patch:
diff --git a/src/openassetio-core/tests/ContextTest.cpp b/src/openassetio-core/tests/ContextTest.cpp
index 2c1e277c8..20e088c15 100644
--- a/src/openassetio-core/tests/ContextTest.cpp
+++ b/src/openassetio-core/tests/ContextTest.cpp
@@ -15,3 +15,11 @@ SCENARIO("Context constructor is private") {
   STATIC_REQUIRE_FALSE(std::is_constructible_v<Context, openassetio::trait::TraitsDataPtr,
                                                openassetio::managerApi::ManagerStateBasePtr>);
 }
+
+SCENARIO("Default construction") {
+  GIVEN("a default constructed Context") {
+    const Context::Ptr context = Context::make();
+
+    THEN("the locale is not null") { CHECK(context->locale); }
+  }
+}
diff --git a/src/openassetio-core/tests/TraitsDataTest.cpp b/src/openassetio-core/tests/TraitsDataTest.cpp
index c8aa8afeb..7cb32efc4 100644
--- a/src/openassetio-core/tests/TraitsDataTest.cpp
+++ b/src/openassetio-core/tests/TraitsDataTest.cpp
@@ -4,6 +4,7 @@
 
 #include <catch2/catch.hpp>
 
+#include <openassetio/errors/exceptions.hpp>
 #include <openassetio/trait/TraitsData.hpp>
 #include <openassetio/trait/collection.hpp>
 #include <openassetio/trait/property.hpp>
@@ -53,4 +54,13 @@ SCENARIO("TraitsData make from other creates a deep copy") {
       }
     }
   }
+  GIVEN("a null TraitsDataPtr") {
+    const TraitsDataPtr nullTraitsData{};
+
+    THEN("attempting to make a copy results in an InputValidationException") {
+      namespace errors = openassetio::errors;
+      CHECK_THROWS_MATCHES(TraitsData::make(nullTraitsData), errors::InputValidationException,
+                           Catch::Message("Cannot copy-construct from a null TraitsData"));
+    }
+  }
 }
diff --git a/src/openassetio-python/tests/package/test_context.py b/src/openassetio-python/tests/package/test_context.py
index 14aad5a2c..f9ed7def6 100644
--- a/src/openassetio-python/tests/package/test_context.py
+++ b/src/openassetio-python/tests/package/test_context.py
@@ -28,8 +28,10 @@
 class Test_Context_init:
     def test_when_constructed_with_no_args_then_has_default_configuration(self):
         context = Context()
-        assert context.locale is None
+        assert isinstance(context.locale, TraitsData)
         assert context.managerState is None
+        # Ensure we're not re-using the same instance.
+        assert Context().locale is not context.locale
 
     def test_when_constructed_with_args_then_has_configuration_from_args(self):
         class TestState(managerApi.ManagerStateBase):
@@ -44,6 +46,13 @@ class TestState(managerApi.ManagerStateBase):
         assert a_context.managerState is expected_state
         assert isinstance(a_context.managerState, TestState)
 
+    def test_when_constructed_with_null_locale_then_raises(self):
+        class TestState(managerApi.ManagerStateBase):
+            pass
+
+        with pytest.raises(TypeError, match="incompatible constructor arguments"):
+            Context(None, TestState())
+
 
 class Test_Context_locale:
     def test_when_set_to_unknown_value_then_raises_ValueError(self, a_context):
diff --git a/src/openassetio-python/tests/package/test_traitsdata.py b/src/openassetio-python/tests/package/test_traitsdata.py
index bf5d0f13e..d4deb4c79 100644
--- a/src/openassetio-python/tests/package/test_traitsdata.py
+++ b/src/openassetio-python/tests/package/test_traitsdata.py
@@ -20,6 +20,10 @@ class _(TraitsData):
 
 
 class Test_TraitsData_Copy_Constructor:
+    def test_when_source_is_None_then_raises(self):
+        with pytest.raises(TypeError, match="incompatible constructor arguments"):
+            TraitsData(None)
+
     def test_when_copying_then_deep_copy_is_made(self):
         data_a = TraitsData()
         data_a.setTraitProperty("a", "p", 1)