From 3538f85723a3dc7a7d9b3d05d166da0d735491c0 Mon Sep 17 00:00:00 2001
From: Tobias Stark <tobias.stark@apex.ai>
Date: Thu, 8 Dec 2022 13:17:15 +0000
Subject: [PATCH] iox-#1821 Check for incompatible flags in SharedMemoryBuilder

Signed-off-by: Tobias Stark <tobias.stark@apex.ai>
---
 .../release-notes/iceoryx-unreleased.md       |  2 ++
 .../shared_memory_object/shared_memory.hpp    |  1 +
 .../shared_memory_object/shared_memory.cpp    | 27 ++++++++++++-------
 .../moduletests/test_posix_shared_memory.cpp  | 14 ++++++++++
 4 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/doc/website/release-notes/iceoryx-unreleased.md b/doc/website/release-notes/iceoryx-unreleased.md
index 20c8ea8fd0e..c8640ea37be 100644
--- a/doc/website/release-notes/iceoryx-unreleased.md
+++ b/doc/website/release-notes/iceoryx-unreleased.md
@@ -59,6 +59,8 @@
 - Remove `cxx::unique_ptr::reset` [\#1655](https://github.com/eclipse-iceoryx/iceoryx/issues/1655)
 - CI uses outdated clang-format [\#1736](https://github.com/eclipse-iceoryx/iceoryx/issues/1736)
 - Avoid UB when accessing `iox::expected` [\#1750](https://github.com/eclipse-iceoryx/iceoryx/issues/1750)
+- Provide a better error message when attempting to create a shared memory in read-only mode
+  [\#1821](https://github.com/eclipse-iceoryx/iceoryx/issues/1821)
 
 **Refactoring:**
 
diff --git a/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp b/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp
index 6187d7d1aed..5f8960c9e34 100644
--- a/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp
+++ b/iceoryx_hoofs/include/iceoryx_hoofs/internal/posix_wrapper/shared_memory_object/shared_memory.hpp
@@ -47,6 +47,7 @@ enum class SharedMemoryError
     NO_FILE_RESIZE_SUPPORT,
     NO_RESIZE_SUPPORT,
     INVALID_FILEDESCRIPTOR,
+    INCOMPATIBLE_OPEN_AND_ACCESS_MODE,
     UNKNOWN_ERROR
 };
 
diff --git a/iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp b/iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp
index 17635d8ec0e..4500955b5d8 100644
--- a/iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp
+++ b/iceoryx_hoofs/source/posix_wrapper/shared_memory_object/shared_memory.cpp
@@ -67,6 +67,16 @@ cxx::expected<SharedMemory, SharedMemoryError> SharedMemoryBuilder::create() noe
 
     auto nameWithLeadingSlash = addLeadingSlash(m_name);
 
+    bool hasOwnership = (m_openMode == OpenMode::EXCLUSIVE_CREATE || m_openMode == OpenMode::PURGE_AND_CREATE
+                         || m_openMode == OpenMode::OPEN_OR_CREATE);
+
+    if (hasOwnership && (m_accessMode == AccessMode::READ_ONLY))
+    {
+        std::cerr << "Cannot create shared-memory file \"" << m_name << "\" in read-only mode. "
+                  << "Initializing a new file requires write access" << std::endl;
+        return cxx::error<SharedMemoryError>(SharedMemoryError::INCOMPATIBLE_OPEN_AND_ACCESS_MODE);
+    }
+
     // the mask will be applied to the permissions, therefore we need to set it to 0
     int sharedMemoryFileHandle = SharedMemory::INVALID_HANDLE;
     mode_t umaskSaved = umask(0U);
@@ -96,27 +106,24 @@ cxx::expected<SharedMemory, SharedMemoryError> SharedMemoryBuilder::create() noe
             // ownership and we just try to open it
             if (m_openMode == OpenMode::OPEN_OR_CREATE && result.get_error().errnum == EEXIST)
             {
+                hasOwnership = false;
                 result = posixCall(iox_shm_open)(nameWithLeadingSlash.c_str(),
                                                  convertToOflags(m_accessMode, OpenMode::OPEN_EXISTING),
                                                  static_cast<mode_t>(m_filePermissions))
                              .failureReturnValue(SharedMemory::INVALID_HANDLE)
                              .evaluate();
-                if (!result.has_error())
-                {
-                    constexpr bool HAS_NO_OWNERSHIP = false;
-                    sharedMemoryFileHandle = result->value;
-                    return cxx::success<SharedMemory>(SharedMemory(m_name, sharedMemoryFileHandle, HAS_NO_OWNERSHIP));
-                }
             }
 
-            printError();
-            return cxx::error<SharedMemoryError>(SharedMemory::errnoToEnum(result.get_error().errnum));
+            // Check again, as the if-block above may have changed `result`
+            if (result.has_error())
+            {
+                printError();
+                return cxx::error<SharedMemoryError>(SharedMemory::errnoToEnum(result.get_error().errnum));
+            }
         }
         sharedMemoryFileHandle = result->value;
     }
 
-    const bool hasOwnership = (m_openMode == OpenMode::EXCLUSIVE_CREATE || m_openMode == OpenMode::PURGE_AND_CREATE
-                               || m_openMode == OpenMode::OPEN_OR_CREATE);
     if (hasOwnership)
     {
         auto result = posixCall(ftruncate)(sharedMemoryFileHandle, static_cast<int64_t>(m_size))
diff --git a/iceoryx_hoofs/test/moduletests/test_posix_shared_memory.cpp b/iceoryx_hoofs/test/moduletests/test_posix_shared_memory.cpp
index 45df400494d..2cb166efa01 100644
--- a/iceoryx_hoofs/test/moduletests/test_posix_shared_memory.cpp
+++ b/iceoryx_hoofs/test/moduletests/test_posix_shared_memory.cpp
@@ -256,5 +256,19 @@ TEST_F(SharedMemory_Test, OpenFailsWhenShmDoesNotExist)
     ASSERT_TRUE(sut.has_error());
 }
 
+TEST_F(SharedMemory_Test, OpenFailsWhenCreatingShmInReadOnlyMode)
+{
+    ::testing::Test::RecordProperty("TEST_ID", "80684160-b243-4ca1-b285-118d2ef36108");
+    auto sut = iox::posix::SharedMemoryBuilder()
+                   .name("readOnlyShmMem")
+                   .size(100)
+                   .accessMode(iox::posix::AccessMode::READ_ONLY)
+                   .openMode(iox::posix::OpenMode::PURGE_AND_CREATE)
+                   .create();
+
+    ASSERT_TRUE(sut.has_error());
+    ASSERT_THAT(sut.get_error(), Eq(SharedMemoryError::INCOMPATIBLE_OPEN_AND_ACCESS_MODE));
+}
+
 
 } // namespace