Merge pull request #6 from leducp/sanitize_shm_name

Sanitize SHM name to ensure valid POSIX path

Author: trns1997

CMakeLists.txt

@@ -45,6 +45,7 @@ endif()
 add_library(kickmsg STATIC
     src/types.cc
     src/Hash.cc
+    src/Naming.cc
     src/Publisher.cc
     src/Subscriber.cc
     src/Region.cc

include/kickmsg/Naming.h

@@ -0,0 +1,34 @@
+#ifndef KICKMSG_NAMING_H
+#define KICKMSG_NAMING_H
+
+#include <string>
+#include <string_view>
+
+namespace kickmsg
+{
+    /// Sanitize a user-supplied name component (namespace / topic / channel /
+    /// owner / tag) into something POSIX shm_open will accept.
+    ///
+    /// POSIX requires shm names to start with a single '/' and contain no
+    /// further '/' characters; Linux additionally constrains the remainder
+    /// to a single path component under /dev/shm.  This helper produces the
+    /// "after the leading slash" fragment for one component — callers
+    /// assemble the final "/<prefix>_<topic>" path themselves.
+    ///
+    /// Rules (human-readable, no hashing):
+    ///   - strip leading '/'  — lets callers pass ROS-style absolute names
+    ///     like "/robot/arm" without producing "//…" or embedded slashes
+    ///   - interior '/' becomes '.' — preserves hierarchy visually
+    ///     ("robot/arm/joint1" -> "robot.arm.joint1")
+    ///   - POSIX "portable filename" chars [A-Za-z0-9._-] pass through
+    ///   - everything else becomes '_' — deterministic, no collisions
+    ///     between benign inputs, still eyeballable in `ls /dev/shm`
+    ///
+    /// Throws std::invalid_argument if the result would be empty (e.g. input
+    /// is "", "/", "///") — a blank component would produce ambiguous names
+    /// like "/prefix_" that silently collide across callers.  \p what is
+    /// interpolated into the exception message ("namespace", "topic", etc.).
+    std::string sanitize_shm_component(std::string_view s, char const* what);
+}
+
+#endif

include/kickmsg/Node.h

@@ -28,6 +28,14 @@ namespace kickmsg
     class Node
     {
     public:
+        // Name components (node name, namespace/prefix, topic, channel,
+        // owner, tag) are sanitized into a POSIX-shm-compatible form:
+        // leading '/' is stripped, interior '/' becomes '.', and any char
+        // outside [A-Za-z0-9._-] becomes '_'. This lets callers pass
+        // ROS-style paths like "/robot/arm/joint1" directly — the region
+        // ends up at "/<prefix>_robot.arm.joint1" in /dev/shm, still
+        // human-readable (no hashing). A component that sanitizes to the
+        // empty string throws std::invalid_argument.
         Node(std::string const& name, std::string const& prefix = "kickmsg");
 
         // Explicit non-copyable / move-only.  Node already holds SharedRegion

src/Naming.cc

@@ -0,0 +1,44 @@
+#include "kickmsg/Naming.h"
+
+#include <cctype>
+#include <stdexcept>
+#include <string>
+
+namespace kickmsg
+{
+    std::string sanitize_shm_component(std::string_view s, char const* what)
+    {
+        std::string out;
+        out.reserve(s.size());
+        bool leading = true;
+        for (char c : s)
+        {
+            if (leading && c == '/')
+            {
+                continue;  // strip any leading '/' (including repeats)
+            }
+            leading = false;
+            if (c == '/')
+            {
+                out.push_back('.');
+            }
+            else if (std::isalnum(static_cast<unsigned char>(c))
+                     || c == '_' || c == '-' || c == '.')
+            {
+                out.push_back(c);
+            }
+            else
+            {
+                out.push_back('_');
+            }
+        }
+        if (out.empty())
+        {
+            throw std::invalid_argument(
+                std::string("kickmsg::sanitize_shm_component: ") + what
+                + " name is empty after sanitization (input: '"
+                + std::string(s) + "')");
+        }
+        return out;
+    }
+}

src/Node.cc

@@ -1,10 +1,12 @@
 #include "kickmsg/Node.h"
 
+#include "kickmsg/Naming.h"
+
 namespace kickmsg
 {
     Node::Node(std::string const& name, std::string const& prefix)
-        : name_{name}
-        , prefix_{prefix}
+        : name_{sanitize_shm_component(name, "node")}
+        , prefix_{sanitize_shm_component(prefix, "namespace")}
     {
     }
 
@@ -136,17 +138,22 @@ namespace kickmsg
 
     std::string Node::make_topic_name(char const* topic) const
     {
-        return "/" + prefix_ + "_" + topic;
+        // prefix_ is pre-sanitized in the ctor; topic is user-supplied on
+        // each call and may be a ROS-style "/a/b/c" path.
+        return "/" + prefix_ + "_" + sanitize_shm_component(topic, "topic");
     }
 
     std::string Node::make_broadcast_name(char const* channel) const
     {
-        return "/" + prefix_ + "_broadcast_" + channel;
+        return "/" + prefix_ + "_broadcast_"
+             + sanitize_shm_component(channel, "channel");
     }
 
     std::string Node::make_mailbox_name(char const* owner, char const* tag) const
     {
-        return "/" + prefix_ + "_" + owner + "_mbx_" + tag;
+        return "/" + prefix_ + "_"
+             + sanitize_shm_component(owner, "mailbox owner") + "_mbx_"
+             + sanitize_shm_component(tag, "mailbox tag");
     }
 
     SharedRegion* Node::find_region(std::string const& shm_name)

tests/CMakeLists.txt

@@ -3,6 +3,7 @@ add_executable(kickmsg_unit
     unit/region-t.cc
     unit/publisher-t.cc
     unit/subscriber-t.cc
+    unit/naming-t.cc
     unit/node-t.cc
 )
 target_link_libraries(kickmsg_unit PRIVATE kickmsg GTest::gmock_main)

tests/unit/naming-t.cc

@@ -0,0 +1,119 @@
+#include <stdexcept>
+#include <string>
+
+#include <gtest/gtest.h>
+
+#include "kickmsg/Naming.h"
+
+using kickmsg::sanitize_shm_component;
+
+// --- Pass-through for already-valid inputs -------------------------------
+
+TEST(SanitizeShmComponent, PlainAlnumIsUnchanged)
+{
+    EXPECT_EQ(sanitize_shm_component("topic",     "topic"), "topic");
+    EXPECT_EQ(sanitize_shm_component("sensor42",  "topic"), "sensor42");
+}
+
+TEST(SanitizeShmComponent, PortableFilenameCharsSurvive)
+{
+    // POSIX portable filename set [A-Za-z0-9._-] must pass through verbatim
+    // — this is what allows `ls /dev/shm` output to stay human-readable.
+    EXPECT_EQ(sanitize_shm_component("a.b_c-d.42", "topic"), "a.b_c-d.42");
+}
+
+// --- Leading slash handling (ROS-style absolute names) -------------------
+
+TEST(SanitizeShmComponent, StripsSingleLeadingSlash)
+{
+    EXPECT_EQ(sanitize_shm_component("/topic", "topic"), "topic");
+}
+
+TEST(SanitizeShmComponent, StripsRepeatedLeadingSlashes)
+{
+    // "///a" -> "a" (not ".a" or "..a") — otherwise a user who typed "//x"
+    // by accident would silently get a different region than "/x".
+    EXPECT_EQ(sanitize_shm_component("///topic", "topic"), "topic");
+}
+
+// --- Interior slash becomes '.' ------------------------------------------
+
+TEST(SanitizeShmComponent, InteriorSlashBecomesDot)
+{
+    EXPECT_EQ(sanitize_shm_component("robot/arm",        "topic"),
+              "robot.arm");
+    EXPECT_EQ(sanitize_shm_component("robot/arm/joint1", "topic"),
+              "robot.arm.joint1");
+}
+
+TEST(SanitizeShmComponent, LeadingAndInteriorSlashesCombine)
+{
+    // The leading-slash strip must not fire for interior slashes after
+    // real characters have been seen.
+    EXPECT_EQ(sanitize_shm_component("/robot/arm/joint1", "topic"),
+              "robot.arm.joint1");
+}
+
+// --- Fallback mapping for everything else --------------------------------
+
+TEST(SanitizeShmComponent, InvalidCharsBecomeUnderscore)
+{
+    EXPECT_EQ(sanitize_shm_component("hello world",  "topic"), "hello_world");
+    EXPECT_EQ(sanitize_shm_component("a:b;c",        "topic"), "a_b_c");
+    EXPECT_EQ(sanitize_shm_component("with\ttab",    "topic"), "with_tab");
+    EXPECT_EQ(sanitize_shm_component("weird@name!",  "topic"), "weird_name_");
+}
+
+TEST(SanitizeShmComponent, UnicodeBytesBecomeUnderscores)
+{
+    // Multi-byte UTF-8 sequences: each byte fails isalnum and maps to '_'.
+    // We don't care about exact width — only that the output stays within
+    // the portable set. "é" is 0xC3 0xA9 -> "__".
+    auto out = sanitize_shm_component("caf\xC3\xA9", "topic");
+    EXPECT_EQ(out, "caf__");
+}
+
+// --- Idempotency ---------------------------------------------------------
+
+TEST(SanitizeShmComponent, Idempotent)
+{
+    // Sanitize twice -> same as sanitize once.  Matters because Node
+    // sanitizes the prefix in the ctor, and make_topic_name could
+    // conceivably be called with the prefix as an input somewhere.
+    auto once  = sanitize_shm_component("/robot/arm joint1", "topic");
+    auto twice = sanitize_shm_component(once, "topic");
+    EXPECT_EQ(once,  "robot.arm_joint1");
+    EXPECT_EQ(twice, once);
+}
+
+// --- Error cases ---------------------------------------------------------
+
+TEST(SanitizeShmComponent, EmptyInputThrows)
+{
+    EXPECT_THROW(sanitize_shm_component("", "topic"), std::invalid_argument);
+}
+
+TEST(SanitizeShmComponent, OnlySlashesThrows)
+{
+    // After stripping leading '/'s there is nothing left — same category
+    // of error as an empty input.
+    EXPECT_THROW(sanitize_shm_component("/",   "topic"), std::invalid_argument);
+    EXPECT_THROW(sanitize_shm_component("///", "topic"), std::invalid_argument);
+}
+
+TEST(SanitizeShmComponent, WhatIsIncludedInErrorMessage)
+{
+    // The `what` label is the only way the caller can tell which of the
+    // several calls in make_topic_name / make_mailbox_name failed — this
+    // test pins that contract so refactors don't accidentally drop it.
+    try
+    {
+        sanitize_shm_component("", "namespace");
+        FAIL() << "expected std::invalid_argument";
+    }
+    catch (std::invalid_argument const& e)
+    {
+        std::string msg = e.what();
+        EXPECT_NE(msg.find("namespace"), std::string::npos) << msg;
+    }
+}

tests/unit/node-t.cc

@@ -294,6 +294,39 @@ TEST_F(NodeTest, SubscribeOrCreateTwiceReusesSameRegion)
     EXPECT_STREQ(schema_view->name, "shared/Type");
 }
 
+TEST_F(NodeTest, RosStyleTopicNamesAreSanitizedIntoShmPath)
+{
+    // End-to-end check that Node feeds topic/namespace through
+    // sanitize_shm_component: ROS-style absolute paths with interior '/'
+    // must round-trip into a POSIX-valid "/<prefix>_robot.arm.joint1"
+    // region, reachable by a peer that passes the same raw topic string.
+    track("/test.ns_robot.arm.joint1");
+
+    kickmsg::Node pub_node("drv", "/test/ns");
+    auto pub = pub_node.advertise("/robot/arm/joint1", small_cfg());
+
+    kickmsg::Node sub_node("log", "/test/ns");
+    auto sub = sub_node.subscribe("/robot/arm/joint1");
+
+    uint32_t val = 7;
+    ASSERT_GE(pub.send(&val, sizeof(val)), 0);
+    auto got = sub.try_receive();
+    ASSERT_TRUE(got.has_value());
+    EXPECT_EQ(std::memcmp(got->data(), &val, sizeof(val)), 0);
+
+    // Also confirm Node::name() / prefix() return the sanitized form so
+    // callers can log/introspect the actual identifiers in use.
+    EXPECT_EQ(pub_node.prefix(), "test.ns");
+    EXPECT_EQ(pub_node.name(),   "drv");
+}
+
+TEST_F(NodeTest, EmptyTopicNameThrows)
+{
+    kickmsg::Node node("n", "test");
+    EXPECT_THROW(node.advertise("", small_cfg()), std::invalid_argument);
+    EXPECT_THROW(node.advertise("/",  small_cfg()), std::invalid_argument);
+}
+
 TEST_F(NodeTest, MailboxMultipleWriters)
 {
     track("/test_owner_mbx_inbox");