URL: https://github.com/freeipa/freeipa/pull/2318
Author: flo-renaud
 Title: #2318: [Backport][ipa-4-7] Clear next field when returnining list 
elements in queue.c
Action: opened

PR body:
"""
This PR was opened automatically because PR #2283 was pushed to master and 
backport to ipa-4-7 is required.
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/2318/head:pr2318
git checkout pr2318
From 39b037c1f8e56452086c2c20569c3ade53338333 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Wed, 22 Aug 2018 15:32:16 -0400
Subject: [PATCH 1/2] Clear next field when returnining list elements in
 queue.c

The ipa-otpd code occasionally removes elements from one queue,
inspects and modifies them, and then inserts them into
another (possibly identical, possibly different) queue.  When the next
pointer isn't cleared, this can result in element membership in both
queues, leading to double frees, or even self-referential elements,
causing infinite loops at traversal time.

Rather than eliminating the pattern, make it safe by clearing the next
field any time an element enters or exits a queue.

Related https://pagure.io/freeipa/issue/7262
---
 daemons/ipa-otpd/queue.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/daemons/ipa-otpd/queue.c b/daemons/ipa-otpd/queue.c
index 9e29fb238d..2944b7ea0d 100644
--- a/daemons/ipa-otpd/queue.c
+++ b/daemons/ipa-otpd/queue.c
@@ -111,6 +111,8 @@ void otpd_queue_push(struct otpd_queue *q, struct otpd_queue_item *item)
         q->head = q->tail = item;
     else
         q->tail = q->tail->next = item;
+
+    item->next = NULL;
 }
 
 void otpd_queue_push_head(struct otpd_queue *q, struct otpd_queue_item *item)
@@ -118,6 +120,8 @@ void otpd_queue_push_head(struct otpd_queue *q, struct otpd_queue_item *item)
     if (item == NULL)
         return;
 
+    item->next = NULL;
+
     if (q->head == NULL)
         q->tail = q->head = item;
     else {
@@ -145,6 +149,8 @@ struct otpd_queue_item *otpd_queue_pop(struct otpd_queue *q)
     if (q->head == NULL)
         q->tail = NULL;
 
+    if (item != NULL)
+        item->next = NULL;
     return item;
 }
 
@@ -160,6 +166,7 @@ struct otpd_queue_item *otpd_queue_pop_msgid(struct otpd_queue *q, int msgid)
             *prev = item->next;
             if (q->head == NULL)
                 q->tail = NULL;
+            item->next = NULL;
             return item;
         }
     }

From dc4c6a1d7b96b665c12d7b54b05060b1397e5185 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharw...@redhat.com>
Date: Thu, 30 Aug 2018 15:34:31 -0400
Subject: [PATCH 2/2] Add cmocka unit tests for ipa otpd queue code

---
 daemons/ipa-otpd/Makefile.am                  |  12 +
 .../ipa-otpd/ipa_otpd_queue_cmocka_tests.c    | 212 ++++++++++++++++++
 2 files changed, 224 insertions(+)
 create mode 100644 daemons/ipa-otpd/ipa_otpd_queue_cmocka_tests.c

diff --git a/daemons/ipa-otpd/Makefile.am b/daemons/ipa-otpd/Makefile.am
index 923e16ebe1..c1d6c20533 100644
--- a/daemons/ipa-otpd/Makefile.am
+++ b/daemons/ipa-otpd/Makefile.am
@@ -21,3 +21,15 @@ ipa_otpd_SOURCES = bind.c forward.c main.c parse.c query.c queue.c stdio.c
 	     $< > $@
 
 CLEANFILES = $(systemdsystemunit_DATA)
+
+TESTS =
+check_PROGRAMS =
+
+if HAVE_CMOCKA
+TESTS += queue_tests
+check_PROGRAMS += queue_tests
+endif
+
+queue_tests_SOURCES = ipa_otpd_queue_cmocka_tests.c queue.c
+queue_tests_CFLAGS = $(CMOCKA_CFLAGS)
+queue_tests_LDADD = $(CMOCKA_LIBS)
diff --git a/daemons/ipa-otpd/ipa_otpd_queue_cmocka_tests.c b/daemons/ipa-otpd/ipa_otpd_queue_cmocka_tests.c
new file mode 100644
index 0000000000..068431e647
--- /dev/null
+++ b/daemons/ipa-otpd/ipa_otpd_queue_cmocka_tests.c
@@ -0,0 +1,212 @@
+/*
+ * FreeIPA 2FA companion daemon - internal queue tests
+ *
+ * Author: Robbie Harwood <rharw...@redhat.com>
+ *
+ * Copyright (C) 2018  Robbie Harwood, Red Hat
+ * see file 'COPYING' for use and warranty information
+ *
+ * This program is free software you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation, either version 3 of the License, or (at your option)
+ * any later version.
+ *
+ * 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 <setjmp.h>
+#include <stdarg.h>
+#include <stddef.h>
+
+#include <cmocka.h>
+
+#include "internal.h"
+
+/* Bypass otpd queue allocation/freeing to avoid calling into LDAP and
+ * krad.  No effort is made to make the types match. */
+static struct otpd_queue_item *new_elt(int id)
+{
+    krb5_error_code ret;
+    struct otpd_queue_item *e = NULL;
+
+    ret = otpd_queue_item_new(NULL, &e);
+    assert_int_equal(ret, 0);
+    assert_ptr_not_equal(e, NULL);
+
+    e->msgid = id;
+    return e;
+}
+static void free_elt(struct otpd_queue_item **e)
+{
+    assert_ptr_not_equal(e, NULL);
+    free(*e);
+    *e = NULL;
+}
+static void free_elts(struct otpd_queue *q)
+{
+    assert_ptr_not_equal(q, NULL);
+    for (struct otpd_queue_item *e = otpd_queue_pop(q); e != NULL;
+         e = otpd_queue_pop(q))
+        free_elt(&e);
+}
+#define otpd_queue_item_new new_elt
+#define otpd_queue_item_free free_elt
+#define otpd_queue_free_items free_elts
+
+static void assert_elt_equal(struct otpd_queue_item *e1,
+                             struct otpd_queue_item *e2)
+{
+    if (e1 == NULL && e2 == NULL)
+        return;
+    assert_ptr_not_equal(e1, NULL);
+    assert_ptr_not_equal(e2, NULL);
+    assert_int_equal(e1->msgid, e2->msgid);
+}
+
+static void test_single_insert()
+{
+    struct otpd_queue q = { NULL };
+    struct otpd_queue_item *ein, *eout;
+
+    ein = new_elt(0);
+    otpd_queue_push(&q, ein);
+
+    eout = otpd_queue_peek(&q);
+    assert_elt_equal(ein, eout);
+
+    eout = otpd_queue_pop(&q);
+    assert_elt_equal(ein, eout);
+    free_elt(&eout);
+
+    eout = otpd_queue_pop(&q);
+    assert_ptr_equal(eout, NULL);
+
+    free_elts(&q);
+}
+
+static void test_jump_insert()
+{
+    struct otpd_queue q = { NULL };
+    struct otpd_queue_item *echeck;
+
+    for (int i = 0; i < 3; i++) {
+        struct otpd_queue_item *e = new_elt(i);
+        otpd_queue_push_head(&q, e);
+
+        echeck = otpd_queue_peek(&q);
+        assert_elt_equal(e, echeck);
+    }
+
+    free_elts(&q);
+}
+
+static void test_garbage_insert()
+{
+    struct otpd_queue q = { NULL };
+    struct otpd_queue_item *e, *g;
+
+    g = new_elt(0);
+    g->next = g;
+    otpd_queue_push(&q, g);
+
+    e = otpd_queue_peek(&q);
+    assert_ptr_equal(e->next, NULL);
+
+    free_elts(&q);
+}
+
+static void test_removal()
+{
+    struct otpd_queue q = { NULL };
+
+    for (int i = 0; i < 3; i++) {
+        struct otpd_queue_item *e = new_elt(i);
+        otpd_queue_push(&q, e);
+    }
+    for (int i = 0; i < 3; i++) {
+        struct otpd_queue_item *e = otpd_queue_pop(&q);
+        assert_ptr_not_equal(e, NULL);
+        assert_ptr_equal(e->next, NULL);
+        assert_int_equal(e->msgid, i);
+        free_elt(&e);
+    }
+}
+
+static void pick_id(struct otpd_queue *q, int msgid)
+{
+    struct otpd_queue_item *e;
+
+    e = otpd_queue_pop_msgid(q, msgid);
+    assert_int_equal(e->msgid, msgid);
+    assert_ptr_equal(e->next, NULL);
+    free_elt(&e);
+    e = otpd_queue_pop_msgid(q, msgid);
+    assert_ptr_equal(e, NULL);
+}
+static void test_pick_removal()
+{
+    struct otpd_queue q = { NULL };
+
+    for (int i = 0; i < 4; i++) {
+        struct otpd_queue_item *e = new_elt(i);
+        otpd_queue_push(&q, e);
+    }
+
+    pick_id(&q, 0); /* first */
+    pick_id(&q, 2); /* middle */
+    pick_id(&q, 3); /* last */
+    pick_id(&q, 1); /* singleton */
+
+    free_elts(&q);
+}
+
+static void test_iter()
+{
+    krb5_error_code ret;
+    struct otpd_queue q = { NULL };
+    const struct otpd_queue *queues[3];
+    struct otpd_queue_iter *iter = NULL;
+    const krad_packet *p = NULL;
+
+    for (ptrdiff_t i = 1; i <= 3; i++) {
+        struct otpd_queue_item *e = new_elt(i);
+        e->req = (void *)i;
+        otpd_queue_push(&q, e);
+    }
+
+    queues[0] = &q;
+    queues[1] = &q;
+    queues[2] = NULL;
+    ret = otpd_queue_iter_new(queues, &iter);
+    assert_int_equal(ret, 0);
+    assert_ptr_not_equal(iter, NULL);
+
+    for (ptrdiff_t i = 0; i < 6; i++) {
+        p = otpd_queue_iter_func(iter, FALSE);
+        assert_ptr_equal(p, (void *) (i % 3 + 1));
+    }
+    p = otpd_queue_iter_func(iter, FALSE);
+    assert_ptr_equal(p, NULL);
+
+    free_elts(&q);
+}
+
+int main(int argc, char *argv[])
+{
+    const struct CMUnitTest tests[] = {
+        cmocka_unit_test(test_single_insert),
+        cmocka_unit_test(test_jump_insert),
+        cmocka_unit_test(test_garbage_insert),
+        cmocka_unit_test(test_removal),
+        cmocka_unit_test(test_pick_removal),
+        cmocka_unit_test(test_iter),
+    };
+
+    return cmocka_run_group_tests(tests, NULL, NULL);
+}
_______________________________________________
FreeIPA-devel mailing list -- freeipa-devel@lists.fedorahosted.org
To unsubscribe send an email to freeipa-devel-le...@lists.fedorahosted.org
Fedora Code of Conduct: https://getfedora.org/code-of-conduct.html
List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines
List Archives: 
https://lists.fedorahosted.org/archives/list/freeipa-devel@lists.fedorahosted.org

Reply via email to