[SSSD] [PATCH] IFP: Add a utility function to reply with an object path

Jakub Hrozek jhrozek at redhat.com
Thu May 15 19:04:34 UTC 2014


On Thu, May 15, 2014 at 08:54:53PM +0200, Lukas Slebodnik wrote:
> You forgot to fix coverity issue
> https://lists.fedorahosted.org/pipermail/sssd-devel/2014-May/019503.html
> 
> LS

I did, thanks.
-------------- next part --------------
>From 4da1f3d1adb1806229ebf04c7bc367d783a57a88 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 11 May 2014 16:15:24 +0200
Subject: [PATCH 1/3] IFP: Add utility functions to escape and unescape object
 paths

D-Bus spec says:
Each element must only contain the ASCII characters "[A-Z][a-z][0-9]_"

This patch adds two utility functions to escape raw input into format
suitable for DBus and conversely transform escaped paths back into raw
paths.
---
 src/responder/ifp/ifp_private.h |   5 +-
 src/responder/ifp/ifpsrv_util.c | 130 ++++++++++++++++++++++++++++++++++++++++
 src/tests/cmocka/test_ifp.c     | 106 ++++++++++++++++++++++++++++++++
 3 files changed, 240 insertions(+), 1 deletion(-)

diff --git a/src/responder/ifp/ifp_private.h b/src/responder/ifp/ifp_private.h
index e7fcaa4c32063a2a387e9b51ca2872431c5246f0..75be038e40334757f7a1889b017f41e6e5c6b989 100644
--- a/src/responder/ifp/ifp_private.h
+++ b/src/responder/ifp/ifp_private.h
@@ -69,9 +69,12 @@ errno_t ifp_req_create(struct sbus_request *dbus_req,
 int ifp_req_create_handle_failure(struct sbus_request *dbus_req, errno_t err);
 
 const char *ifp_path_strip_prefix(const char *path, const char *prefix);
+
+char *ifp_bus_path_unescape(TALLOC_CTX *mem_ctx, const char *path);
+char *ifp_bus_path_escape(TALLOC_CTX *mem_ctx, const char *path);
+
 errno_t ifp_add_ldb_el_to_dict(DBusMessageIter *iter_dict,
                                struct ldb_message_element *el);
-
 const char **ifp_parse_attr_list(TALLOC_CTX *mem_ctx, const char *conf_str);
 bool ifp_attr_allowed(const char *whitelist[], const char *attr);
 #endif /* _IFPSRV_PRIVATE_H_ */
diff --git a/src/responder/ifp/ifpsrv_util.c b/src/responder/ifp/ifpsrv_util.c
index 4ef0be6afe5fc1b638eae69166a40dcd400d7c37..6fcd2247901137d328f8187c120ea1bb57db15f2 100644
--- a/src/responder/ifp/ifpsrv_util.c
+++ b/src/responder/ifp/ifpsrv_util.c
@@ -106,6 +106,136 @@ const char *ifp_path_strip_prefix(const char *path, const char *prefix)
     return NULL;
 }
 
+/* The following path related functions are based on similar code in
+ * storaged, just tailored to use talloc instead of glib
+ */
+char *ifp_bus_path_escape(TALLOC_CTX *mem_ctx, const char *path)
+{
+    size_t n;
+    char *safe_path = NULL;
+    TALLOC_CTX *tmp_ctx = NULL;
+
+    /* The path must be valid */
+    if (path == NULL) {
+        return NULL;
+    }
+
+    tmp_ctx = talloc_new(NULL);
+    if (tmp_ctx == NULL) {
+        return NULL;
+    }
+
+    safe_path = talloc_strdup(tmp_ctx, "");
+    if (safe_path == NULL) {
+        goto done;
+    }
+
+    /* Special case for an empty string */
+    if (strcmp(path, "") == 0) {
+        /* the for loop would just fall through */
+        safe_path = talloc_asprintf_append_buffer(safe_path, "_");
+    }
+
+    for (n = 0; path[n]; n++) {
+        int c = path[n];
+        /* D-Bus spec says:
+         * *
+         * * Each element must only contain the ASCII characters
+         * "[A-Z][a-z][0-9]_"
+         * */
+        if ((c >= 'A' && c <= 'Z')
+                || (c >= 'a' && c <= 'z')
+                || (c >= '0' && c <= '9')) {
+            safe_path = talloc_asprintf_append_buffer(safe_path, "%c", c);
+            if (safe_path == NULL) {
+                goto done;
+            }
+        } else {
+            safe_path = talloc_asprintf_append_buffer(safe_path, "_%02x", c);
+            if (safe_path == NULL) {
+                goto done;
+            }
+        }
+    }
+
+    safe_path = talloc_steal(mem_ctx, safe_path);
+done:
+    talloc_free(tmp_ctx);
+    return safe_path;
+}
+
+static inline int unhexchar(char c)
+{
+    if (c >= '0' && c <= '9') {
+        return c - '0';
+    }
+
+    if (c >= 'a' && c <= 'f') {
+        return c - 'a' + 10;
+    }
+
+    if (c >= 'A' && c <= 'F') {
+        return c - 'A' + 10;
+    }
+
+    return -1;
+}
+
+char *ifp_bus_path_unescape(TALLOC_CTX *mem_ctx, const char *path)
+{
+    char *safe_path;
+    const char *p;
+    int a, b, c;
+    TALLOC_CTX *tmp_ctx = NULL;
+
+    tmp_ctx = talloc_new(NULL);
+    if (tmp_ctx == NULL) {
+        return NULL;
+    }
+
+    safe_path = talloc_strdup(tmp_ctx, "");
+    if (safe_path == NULL) {
+        goto done;
+    }
+
+    /* Special case for the empty string */
+    if (strcmp(path, "_") == 0) {
+        safe_path = talloc_steal(mem_ctx, safe_path);
+        goto done;
+    }
+
+    for (p = path; *p; p++) {
+        if (*p == '_') {
+            /* There must be at least two more chars after underscore */
+            if (p[1] == '\0' || p[2] == '\0') {
+                safe_path = NULL;
+                goto done;
+            }
+
+            if ((a = unhexchar(p[1])) < 0
+                    || (b = unhexchar(p[2])) < 0) {
+                /* Invalid escape code, let's take it literal then */
+                c = '_';
+            } else {
+                c = ((a << 4) | b);
+                p += 2;
+            }
+        } else  {
+            c = *p;
+        }
+
+        safe_path = talloc_asprintf_append_buffer(safe_path, "%c", c);
+        if (safe_path == NULL) {
+            goto done;
+        }
+    }
+
+    safe_path = talloc_steal(mem_ctx, safe_path);
+done:
+    talloc_free(tmp_ctx);
+    return safe_path;
+}
+
 errno_t ifp_add_ldb_el_to_dict(DBusMessageIter *iter_dict,
                                struct ldb_message_element *el)
 {
diff --git a/src/tests/cmocka/test_ifp.c b/src/tests/cmocka/test_ifp.c
index 70ebf942dcb1e366516bed92620dece6dedd0a0d..8d9b8563563d1cb274eb44f044ebc05bb210596f 100644
--- a/src/tests/cmocka/test_ifp.c
+++ b/src/tests/cmocka/test_ifp.c
@@ -28,6 +28,11 @@
 #include "responder/ifp/ifp_private.h"
 #include "sbus/sssd_dbus_private.h"
 
+/* dbus library checks for valid object paths when unit testing, we don't
+ * want that */
+#undef DBUS_TYPE_OBJECT_PATH
+#define DBUS_TYPE_OBJECT_PATH ((int) 's')
+
 static struct ifp_ctx *
 mock_ifp_ctx(TALLOC_CTX *mem_ctx)
 {
@@ -305,6 +310,106 @@ void test_attr_allowed(void **state)
     assert_false(ifp_attr_allowed(NULL, "name"));
 }
 
+void test_path_escape_unescape(void **state)
+{
+    char *escaped;
+    char *raw;
+    TALLOC_CTX *mem_ctx;
+
+    assert_true(leak_check_setup());
+    mem_ctx = talloc_new(global_talloc_context);
+
+    escaped = ifp_bus_path_escape(mem_ctx, "noescape");
+    assert_non_null(escaped);
+    assert_string_equal(escaped, "noescape");
+    raw = ifp_bus_path_unescape(mem_ctx, escaped);
+    talloc_free(escaped);
+    assert_non_null(raw);
+    assert_string_equal(raw, "noescape");
+    talloc_free(raw);
+
+    escaped = ifp_bus_path_escape(mem_ctx, "redhat.com");
+    assert_non_null(escaped);
+    assert_string_equal(escaped, "redhat_2ecom"); /* dot is 0x2E in ASCII */
+    raw = ifp_bus_path_unescape(mem_ctx, escaped);
+    talloc_free(escaped);
+    assert_non_null(raw);
+    assert_string_equal(raw, "redhat.com");
+    talloc_free(raw);
+
+    escaped = ifp_bus_path_escape(mem_ctx, "path_with_underscore");
+    assert_non_null(escaped);
+    /* underscore is 0x5F in ascii */
+    assert_string_equal(escaped, "path_5fwith_5funderscore");
+    raw = ifp_bus_path_unescape(mem_ctx, escaped);
+    talloc_free(escaped);
+    assert_non_null(raw);
+    assert_string_equal(raw, "path_with_underscore");
+    talloc_free(raw);
+
+    /* empty string */
+    escaped = ifp_bus_path_escape(mem_ctx, "");
+    assert_non_null(escaped);
+    assert_string_equal(escaped, "_");
+    raw = ifp_bus_path_unescape(mem_ctx, escaped);
+    talloc_free(escaped);
+    assert_non_null(raw);
+    assert_string_equal(raw, "");
+    talloc_free(raw);
+
+    /* negative tests */
+    escaped = ifp_bus_path_escape(mem_ctx, NULL);
+    assert_null(escaped);
+    raw = ifp_bus_path_unescape(mem_ctx, "wrongpath_");
+    assert_null(raw);
+
+    assert_true(leak_check_teardown());
+}
+
+#define PATH_BASE "/some/path"
+
+struct ifp_test_req_ctx {
+    struct ifp_req *ireq;
+    struct sbus_request *sr;
+    struct ifp_ctx *ifp_ctx;
+};
+
+void ifp_test_req_setup(void **state)
+{
+    struct ifp_test_req_ctx *test_ctx;
+    errno_t ret;
+
+    assert_true(leak_check_setup());
+
+    test_ctx = talloc_zero(global_talloc_context, struct ifp_test_req_ctx);
+    assert_non_null(test_ctx);
+    test_ctx->ifp_ctx = mock_ifp_ctx(test_ctx);
+    assert_non_null(test_ctx->ifp_ctx);
+
+    test_ctx->sr = mock_sbus_request(test_ctx, geteuid());
+    assert_non_null(test_ctx->sr);
+
+    ret = ifp_req_create(test_ctx->sr, test_ctx->ifp_ctx, &test_ctx->ireq);
+    assert_int_equal(ret, EOK);
+    assert_non_null(test_ctx->ireq);
+
+    check_leaks_push(test_ctx);
+    *state = test_ctx;
+}
+
+void ifp_test_req_teardown(void **state)
+{
+    struct ifp_test_req_ctx *test_ctx = talloc_get_type_abort(*state,
+                                                struct ifp_test_req_ctx);
+
+    assert_true(check_leaks_pop(test_ctx) == true);
+
+    dbus_message_unref(test_ctx->sr->message);
+    talloc_free(test_ctx);
+
+    assert_true(leak_check_teardown());
+}
+
 int main(int argc, const char *argv[])
 {
     poptContext pc;
@@ -322,6 +427,7 @@ int main(int argc, const char *argv[])
         unit_test(test_el_to_dict),
         unit_test(test_attr_acl),
         unit_test(test_attr_allowed),
+        unit_test(test_path_escape_unescape),
     };
 
     /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
1.9.0

-------------- next part --------------
>From f1520f61ad84464bf679c08cfd1d1fff3408896d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
Date: Sun, 11 May 2014 16:18:06 +0200
Subject: [PATCH 2/3] IFP: Add a utility function to reply with an object path

---
 src/responder/ifp/ifp_private.h |  6 ++++++
 src/responder/ifp/ifpsrv_util.c | 42 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/src/responder/ifp/ifp_private.h b/src/responder/ifp/ifp_private.h
index 75be038e40334757f7a1889b017f41e6e5c6b989..bf2015a9722ff4b4f831cb63cd528881f0f259a8 100644
--- a/src/responder/ifp/ifp_private.h
+++ b/src/responder/ifp/ifp_private.h
@@ -73,6 +73,12 @@ const char *ifp_path_strip_prefix(const char *path, const char *prefix);
 char *ifp_bus_path_unescape(TALLOC_CTX *mem_ctx, const char *path);
 char *ifp_bus_path_escape(TALLOC_CTX *mem_ctx, const char *path);
 
+char *_ifp_reply_objpath(TALLOC_CTX *mem_ctx, const char *base,
+                         const char *part, ...);
+
+#define ifp_reply_objpath(mem_ctx, base, ...) \
+    _ifp_reply_objpath(mem_ctx, base, ##__VA_ARGS__, NULL)
+
 errno_t ifp_add_ldb_el_to_dict(DBusMessageIter *iter_dict,
                                struct ldb_message_element *el);
 const char **ifp_parse_attr_list(TALLOC_CTX *mem_ctx, const char *conf_str);
diff --git a/src/responder/ifp/ifpsrv_util.c b/src/responder/ifp/ifpsrv_util.c
index 6fcd2247901137d328f8187c120ea1bb57db15f2..c0ab686e4527a2508f67b789151365dfdbb89f25 100644
--- a/src/responder/ifp/ifpsrv_util.c
+++ b/src/responder/ifp/ifpsrv_util.c
@@ -236,6 +236,48 @@ done:
     return safe_path;
 }
 
+char *
+_ifp_reply_objpath(TALLOC_CTX *mem_ctx, const char *base,
+                   const char *part, ...)
+{
+    char *safe_part;
+    char *path = NULL;
+    va_list va;
+
+    if (base == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, "Wrong object path base!\n");
+        return NULL;
+    }
+
+    path = talloc_strdup(mem_ctx, base);
+    if (path == NULL) return NULL;
+
+    va_start(va, part);
+    while (part != NULL) {
+        safe_part = ifp_bus_path_escape(mem_ctx, part);
+        if (safe_part == NULL) {
+            DEBUG(SSSDBG_OP_FAILURE, "Could not add [%s] to objpath\n", part);
+            goto fail;
+        }
+
+        path = talloc_asprintf_append(path, "/%s", safe_part);
+        talloc_free(safe_part);
+        if (path == NULL) {
+            goto fail;
+        }
+
+        part = va_arg(va, const char *);
+    }
+    va_end(va);
+
+    return path;
+
+fail:
+    va_end(va);
+    talloc_free(path);
+    return NULL;
+}
+
 errno_t ifp_add_ldb_el_to_dict(DBusMessageIter *iter_dict,
                                struct ldb_message_element *el)
 {
-- 
1.9.0

-------------- next part --------------
>From 52e1cf07eadadc2b69b8f8cb9ee7b611621590b1 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 11 May 2014 16:18:28 +0200
Subject: [PATCH 3/3] IFP: Add a unit test for ifp_reply_objpath

---
 src/tests/cmocka/test_ifp.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/src/tests/cmocka/test_ifp.c b/src/tests/cmocka/test_ifp.c
index 8d9b8563563d1cb274eb44f044ebc05bb210596f..650a9496978854c4c1aa6b72f58e831d60ca7c10 100644
--- a/src/tests/cmocka/test_ifp.c
+++ b/src/tests/cmocka/test_ifp.c
@@ -410,6 +410,32 @@ void ifp_test_req_teardown(void **state)
     assert_true(leak_check_teardown());
 }
 
+void test_reply_path(void **state)
+{
+    struct ifp_test_req_ctx *test_ctx = talloc_get_type_abort(*state,
+                                                struct ifp_test_req_ctx);
+    char *path;
+
+    /* Doesn't need escaping */
+    path = ifp_reply_objpath(test_ctx->ireq, PATH_BASE, "domname", NULL);
+    assert_non_null(path);
+    assert_string_equal(path, PATH_BASE"/domname");
+    talloc_free(path);
+}
+
+void test_reply_path_escape(void **state)
+{
+    struct ifp_test_req_ctx *test_ctx = talloc_get_type_abort(*state,
+                                                struct ifp_test_req_ctx);
+    char *path;
+
+    /* A dot needs escaping */
+    path = ifp_reply_objpath(test_ctx->ireq, PATH_BASE, "redhat.com", NULL);
+    assert_non_null(path);
+    assert_string_equal(path, PATH_BASE"/redhat_2ecom");
+    talloc_free(path);
+}
+
 int main(int argc, const char *argv[])
 {
     poptContext pc;
@@ -428,6 +454,10 @@ int main(int argc, const char *argv[])
         unit_test(test_attr_acl),
         unit_test(test_attr_allowed),
         unit_test(test_path_escape_unescape),
+        unit_test_setup_teardown(test_reply_path,
+                                 ifp_test_req_setup, ifp_test_req_teardown),
+        unit_test_setup_teardown(test_reply_path_escape,
+                                 ifp_test_req_setup, ifp_test_req_teardown),
     };
 
     /* Set debug level to invalid value so we can deside if -d 0 was used. */
-- 
1.9.0



More information about the sssd-devel mailing list