feedbck: better CopyableWlArray + only running death tests if not asan

src/include/server/mir/scene/text_input_hub.h

@@ -102,13 +102,14 @@ struct TextInputState
 class CopyableWlArray
 {
 public:
-    explicit CopyableWlArray(wl_array* in_data)
-        : data_{copy(in_data)}
+    explicit CopyableWlArray(wl_array const* in_data)
     {
+        wl_array_init(&data_);
+        wl_array_copy(&data_, const_cast<wl_array*>(in_data));
     }
-
+    
     CopyableWlArray(CopyableWlArray const& other)
-        : CopyableWlArray{other.data_}
+        : CopyableWlArray{&other.data_}
     {
     }
 
@@ -120,24 +121,15 @@ class CopyableWlArray
 
     ~CopyableWlArray()
     {
-        wl_array_release(data_);
+        wl_array_release(&data_);
     }
 
     [[nodiscard]] wl_array* data() const
     {
-        return data_;
+        return const_cast<wl_array*>(&data_);
     }
-
 private:
-    static wl_array* copy(wl_array* in_data)
-    {
-        auto data = new wl_array();
-        wl_array_init(data);
-        wl_array_copy(data, in_data);
-        return data;
-    }
-
-    wl_array* data_;
+    wl_array data_;
 };
 
 /// See text-input-unstable-v3.xml for details

tests/unit-tests/CMakeLists.txt

@@ -42,7 +42,11 @@ set_target_properties(
 # Umockdev uses glib, which uses the deprecated "register" allocation specifier
 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Dregister=")
 
-set(UMOCK_UNIT_TEST_SOURCES test_udev_wrapper.cpp)
+if(CMAKE_BUILD_TYPE STREQUAL "AddressSanitizer")
+  set(UMOCK_UNIT_TEST_SOURCES test_udev_wrapper.cpp)
+else()
+  set(UMOCK_UNIT_TEST_SOURCES test_udev_wrapper.cpp test_udev_wrapper_asan_skip.cpp)
+endif()
 
 set(
   UNIT_TEST_SOURCES
@@ -133,8 +137,15 @@ set_property(
   SOURCE input/test_logind_console_services.cpp input/test_input_platform_probing.cpp
   SOURCE input/evdev/test_evdev_input_platform.cpp
   SOURCE graphics/test_platform_prober.cpp
+  SOURCE test_udev_wrapper_asan_skip.cpp
   PROPERTY COMPILE_OPTIONS -Wno-variadic-macros)
 
+if(NOT CMAKE_BUILD_TYPE STREQUAL "AddressSanitizer")
+  set_property(
+    SOURCE test_udev_wrapper_asan_skip.cpp
+    PROPERTY COMPILE_OPTIONS -Wno-variadic-macros)
+endif()
+
 if ("test_touchspot_controller.cpp:array-bounds" IN_LIST MIR_COMPILER_QUIRKS)
   set_property(
     SOURCE input/test_touchspot_controller.cpp

tests/unit-tests/test_udev_wrapper.cpp

@@ -26,33 +26,15 @@
 #include <poll.h>
 #include <sys/sysmacros.h>
 
-#if defined(__has_feature)
-#   if __has_feature(address_sanitizer) // for clang
-#       define __SANITIZE_ADDRESS__ // GCC already sets this
-#   endif
-#endif
-
 namespace mtf=mir_test_framework;
 
 namespace
 {
-#if !defined(__SANITIZE_ADDRESS__)
-bool KilledByInvalidMemoryAccess(int exit_status)
-{
-    return testing::KilledBySignal(SIGSEGV)(exit_status) ||
-           testing::KilledBySignal(SIGBUS)(exit_status) ||
-           testing::KilledBySignal(SIGABRT)(exit_status) ||
-           // It seems that valgrind kills us with SIGKILL
-           testing::KilledBySignal(SIGKILL)(exit_status);
-}
-#endif
-
 class UdevWrapperTest : public ::testing::Test
 {
 public:
     mtf::UdevEnvironment udev_environment;
 };
-
 }
 
 TEST_F(UdevWrapperTest, IteratesOverCorrectNumberOfDevices)
@@ -321,31 +303,6 @@ TEST_F(UdevWrapperTest, EnumeratorAddMatchSysnameIncludesCorrectDevices)
     }
 }
 
-typedef UdevWrapperTest UdevWrapperDeathTest;
-
-#if !defined(__SANITIZE_ADDRESS__)
-TEST_F(UdevWrapperDeathTest, DereferencingEndReturnsInvalidObject)
-{
-    ::testing::FLAGS_gtest_death_test_style = "threadsafe";
-    udev_environment.add_device("drm", "control64D", NULL, {}, {});
-    udev_environment.add_device("drm", "card1", NULL, {}, {});
-
-    mir::udev::Enumerator devices(std::make_shared<mir::udev::Context>());
-
-    devices.scan_devices();
-
-    MIR_EXPECT_EXIT((*devices.end()).subsystem(), KilledByInvalidMemoryAccess, "");
-
-    auto iter = devices.begin();
-
-    while(iter != devices.end())
-    {
-        iter++;
-    }
-    MIR_EXPECT_EXIT((*iter).subsystem(), KilledByInvalidMemoryAccess, "");
-}
-#endif
-
 TEST_F(UdevWrapperTest, MemberDereferenceWorks)
 {
     udev_environment.add_device("drm", "control64D", NULL, {}, {});
@@ -360,29 +317,6 @@ TEST_F(UdevWrapperTest, MemberDereferenceWorks)
     EXPECT_STREQ("drm", iter->subsystem());
 }
 
-#if !defined(__SANITIZE_ADDRESS__)
-TEST_F(UdevWrapperDeathTest, MemberDereferenceOfEndDies)
-{
-    ::testing::FLAGS_gtest_death_test_style = "threadsafe";
-    udev_environment.add_device("drm", "control64D", NULL, {}, {});
-    udev_environment.add_device("drm", "card1", NULL, {}, {});
-
-    mir::udev::Enumerator devices(std::make_shared<mir::udev::Context>());
-
-    devices.scan_devices();
-
-    MIR_EXPECT_EXIT(devices.end()->subsystem(), KilledByInvalidMemoryAccess, "");
-
-    auto iter = devices.begin();
-
-    while(iter != devices.end())
-    {
-        iter++;
-    }
-    MIR_EXPECT_EXIT(iter->subsystem(), KilledByInvalidMemoryAccess, "");
-}
-#endif
-
 TEST_F(UdevWrapperTest, UdevMonitorDoesNotTriggerBeforeEnabling)
 {
     mir::udev::Monitor monitor{mir::udev::Context()};

tests/unit-tests/test_udev_wrapper_asan_skip.cpp

@@ -0,0 +1,88 @@
+/*
+ * Copyright © Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 or 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "mir_test_framework/udev_environment.h"
+#include "mir/udev/wrapper.h"
+#include "mir/test/death.h"
+
+#include <gtest/gtest.h>
+#include <memory>
+#include <stdexcept>
+#include <libudev.h>
+
+namespace mtf=mir_test_framework;
+
+namespace
+{
+bool KilledByInvalidMemoryAccess(int exit_status)
+{
+    return testing::KilledBySignal(SIGSEGV)(exit_status) ||
+           testing::KilledBySignal(SIGBUS)(exit_status) ||
+           testing::KilledBySignal(SIGABRT)(exit_status) ||
+           // It seems that valgrind kills us with SIGKILL
+           testing::KilledBySignal(SIGKILL)(exit_status);
+}
+
+/// Tests in this suite are skipped during the address sanitizer build,
+/// as they purposefully do things that the address sanitizer would not like.
+class UdevWrapperDeathTest : public ::testing::Test
+{
+public:
+    mtf::UdevEnvironment udev_environment;
+};
+}
+
+TEST_F(UdevWrapperDeathTest, DereferencingEndReturnsInvalidObject)
+{
+    ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+    udev_environment.add_device("drm", "control64D", NULL, {}, {});
+    udev_environment.add_device("drm", "card1", NULL, {}, {});
+
+    mir::udev::Enumerator devices(std::make_shared<mir::udev::Context>());
+
+    devices.scan_devices();
+
+    MIR_EXPECT_EXIT((*devices.end()).subsystem(), KilledByInvalidMemoryAccess, "");
+
+    auto iter = devices.begin();
+
+    while(iter != devices.end())
+    {
+        iter++;
+    }
+    MIR_EXPECT_EXIT((*iter).subsystem(), KilledByInvalidMemoryAccess, "");
+}
+
+TEST_F(UdevWrapperDeathTest, MemberDereferenceOfEndDies)
+{
+    ::testing::FLAGS_gtest_death_test_style = "threadsafe";
+    udev_environment.add_device("drm", "control64D", NULL, {}, {});
+    udev_environment.add_device("drm", "card1", NULL, {}, {});
+
+    mir::udev::Enumerator devices(std::make_shared<mir::udev::Context>());
+
+    devices.scan_devices();
+
+    MIR_EXPECT_EXIT(devices.end()->subsystem(), KilledByInvalidMemoryAccess, "");
+
+    auto iter = devices.begin();
+
+    while(iter != devices.end())
+    {
+        iter++;
+    }
+    MIR_EXPECT_EXIT(iter->subsystem(), KilledByInvalidMemoryAccess, "");
+}