Fix multiple setting in DTLS
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 3 Aug 2016 23:14:30 +0000 (16:14 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Thu, 4 Aug 2016 21:09:53 +0000 (14:09 -0700)
The bug was that __get_dtls was returning a v->dtls, not a v.  Thus if
we set twice, we'd dereference the *value*, instead of v.  In essence,
we'd set v->dtls->dtls = dtls.  That's a wild write.

Incidentally, our DTLS handles set/get calls from within the destructor
without resorting to infinite loops, PTHREAD_DESTRUCTOR_ITERATIONS (4)
retries, or other nasty shit.  Nicely done, Kevin.  =)

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
user/parlib/dtls.c
user/utest/dtls.c [new file with mode: 0644]

index 157517d..52c9f18 100644 (file)
@@ -142,7 +142,8 @@ void dtls_key_delete(dtls_key_t key)
        __maybe_free_dtls_key(key);
 }
 
-static inline void *__get_dtls(dtls_data_t *dtls_data, dtls_key_t key)
+static inline struct dtls_value *__get_dtls(dtls_data_t *dtls_data,
+                                            dtls_key_t key)
 {
        struct dtls_value *v;
 
@@ -150,11 +151,11 @@ static inline void *__get_dtls(dtls_data_t *dtls_data, dtls_key_t key)
        if (key->id < NUM_STATIC_KEYS) {
                v = &dtls_data->early_values[key->id];
                if (v->key != NULL)
-                       return v->dtls;
+                       return v;
        } else {
                TAILQ_FOREACH(v, &dtls_data->list, link)
                        if (v->key == key)
-                               return v->dtls;
+                               return v;
        }
        return NULL;
 }
@@ -225,11 +226,13 @@ void set_dtls(dtls_key_t key, void *dtls)
 void *get_dtls(dtls_key_t key)
 {
        dtls_data_t *dtls_data = NULL;
+       struct dtls_value *v;
 
        if (!__dtls_initialized)
                return NULL;
        dtls_data = &__dtls_data;
-       return __get_dtls(dtls_data, key);
+       v = __get_dtls(dtls_data, key);
+       return v ? v->dtls : NULL;
 }
 
 void destroy_dtls(void)
diff --git a/user/utest/dtls.c b/user/utest/dtls.c
new file mode 100644 (file)
index 0000000..294f378
--- /dev/null
@@ -0,0 +1,109 @@
+/* Copyright (c) 2016 Google, Inc.
+ * Barret Rhoden <brho@cs.berkeley.edu>
+ * See LICENSE for details. */
+
+#include <utest/utest.h>
+#include <parlib/dtls.h>
+#include <stdio.h>
+
+TEST_SUITE("DTLS");
+
+/* <--- Begin definition of test cases ---> */
+
+bool test_get_without_set(void)
+{
+       dtls_key_t key;
+       void *got_val;
+
+       key = dtls_key_create(0);
+       got_val = get_dtls(key);
+       UT_ASSERT_FMT("Expected 0, got %p", got_val == 0, 0, got_val);
+       destroy_dtls();
+       return TRUE;
+}
+
+/* This catches a bug, but it had to have a set happen at some point. */
+bool test_get_after_reset(void)
+{
+       dtls_key_t key;
+       void *set_val = (void*)0x15;
+       void *got_val;
+
+       key = dtls_key_create(0);
+       set_dtls(key, set_val);
+       destroy_dtls();
+
+       key = dtls_key_create(0);
+       got_val = get_dtls(key);
+       UT_ASSERT_FMT("Expected 0, got %p", got_val == 0, 0, got_val);
+       destroy_dtls();
+       return TRUE;
+}
+
+bool test_set_and_get(void)
+{
+       dtls_key_t key;
+       void *set_val = (void*)0x15;
+       void *got_val;
+
+       key = dtls_key_create(0);
+       set_dtls(key, set_val);
+       got_val = get_dtls(key);
+       UT_ASSERT_FMT("Expected %p, got %p", got_val == set_val, set_val, got_val);
+       destroy_dtls();
+       return TRUE;
+}
+
+bool test_set_twice(void)
+{
+       dtls_key_t key;
+       void *set_val = (void*)0x15;
+       void *got_val;
+
+       key = dtls_key_create(0);
+       set_dtls(key, set_val);
+       set_dtls(key, set_val + 1);
+       destroy_dtls();
+       return TRUE;
+}
+
+static dtls_key_t sfd_global_key;
+
+static void setting_dtor(void *arg)
+{
+       set_dtls(sfd_global_key, (void*)0xf00);
+}
+
+/* Users can set from a destructor.  In some implementations of pthread keys,
+ * you can't safely set_specific from within a destructor without the risk of an
+ * infinite loop or storage loss.  Our DTLS implementation shouldn't be doing
+ * either, though we can't check for storage loss. */
+bool test_set_from_dtor(void)
+{
+       void *set_val = (void*)0x15;
+
+       sfd_global_key = dtls_key_create(setting_dtor);
+       set_dtls(sfd_global_key, set_val);
+       destroy_dtls();
+       return TRUE;
+}
+
+/* <--- End definition of test cases ---> */
+
+struct utest utests[] = {
+       UTEST_REG(get_without_set),
+       UTEST_REG(get_after_reset),
+       UTEST_REG(set_and_get),
+       UTEST_REG(set_twice),
+       UTEST_REG(set_from_dtor),
+};
+int num_utests = sizeof(utests) / sizeof(struct utest);
+
+int main(int argc, char *argv[])
+{
+       // Run test suite passing it all the args as whitelist of what tests to run.
+       char **whitelist = &argv[1];
+       int whitelist_len = argc - 1;
+
+       RUN_TEST_SUITE(utests, num_utests, whitelist, whitelist_len);
+}