[#253] Fix reference count leak in add_option

RELNOTES
    Added release note

common/options.c
    add_option() - always dereference the looked up option

common/tests/option_unittest.c
    Added new unit test: ATF_TC_BODY(add_option_ref_cnt, tc)

diff --git a/RELNOTES b/RELNOTES
index 64d45b204a8af9f1b386a07f687cf42c06a562e5..455027a516cc6dbd6b7d0dfaeb6cc3389a17d7dc 100644 (file)
--- a/RELNOTES
+++ b/RELNOTES
@@ -28,6 +28,13 @@ ISC DHCP is open source software maintained by Internet Systems
 Consortium.  This product includes cryptographic software written
 by Eric Young (eay@cryptsoft.com).
 
+               Changes since 4.4.3 (New Features)
+
+! Corrected a reference count leak that occurs when the server builds
+  responses to leasequery packets.
+  [Gitblab #253]
+  CVE: <TBD>
+
                Changes since 4.4.2-P1 (New Features)
 
 - Two new OMAPI function calls were added, `dhcpctl_timed_connect()`

diff --git a/common/options.c b/common/options.c
index 92c8fee6bc6816a825f384b35ae3b27c50d941da..f0959cb2b03a410a053405adb8971f1312dd2f0e 100644 (file)
--- a/common/options.c
+++ b/common/options.c
@@ -4452,6 +4452,8 @@ add_option(struct option_state *options,
        if (!option_cache_allocate(&oc, MDL)) {
                log_error("No memory for option cache adding %s (option %d).",
                          option->name, option_num);
+               /* Get rid of reference created during hash lookup. */
+               option_dereference(&option, MDL);
                return 0;
        }
 
@@ -4463,6 +4465,8 @@ add_option(struct option_state *options,
                             MDL)) {
                log_error("No memory for constant data adding %s (option %d).",
                          option->name, option_num);
+               /* Get rid of reference created during hash lookup. */
+               option_dereference(&option, MDL);
                option_cache_dereference(&oc, MDL);
                return 0;
        }
@@ -4471,6 +4475,9 @@ add_option(struct option_state *options,
        save_option(&dhcp_universe, options, oc);
        option_cache_dereference(&oc, MDL);
 
+       /* Get rid of reference created during hash lookup. */
+       option_dereference(&option, MDL);
+
        return 1;
 }
 

diff --git a/common/tests/option_unittest.c b/common/tests/option_unittest.c
index 600ebe6005d60caeadd3ed82d7389004376f0944..963b56637db0880b32384e5d94e2ebf9dff419e4 100644 (file)
--- a/common/tests/option_unittest.c
+++ b/common/tests/option_unittest.c
@@ -213,6 +213,59 @@ ATF_TC_BODY(parse_X, tc)
     }
 }
 
+ATF_TC(add_option_ref_cnt);
+
+ATF_TC_HEAD(add_option_ref_cnt, tc)
+{
+    atf_tc_set_md_var(tc, "descr",
+        "Verify add_option() does not leak option ref counts.");
+}
+
+ATF_TC_BODY(add_option_ref_cnt, tc)
+{
+    struct option_state *options = NULL;
+    struct option *option = NULL;
+    unsigned int cid_code = DHO_DHCP_CLIENT_IDENTIFIER;
+    char *cid_str = "1234";
+    int refcnt_before = 0;
+
+    // Look up the option we're going to add.
+    initialize_common_option_spaces();
+    if (!option_code_hash_lookup(&option, dhcp_universe.code_hash,
+                                 &cid_code, 0, MDL)) {
+        atf_tc_fail("cannot find option definition?");
+    }
+
+    // Get the option's reference count before we call add_options.
+    refcnt_before = option->refcnt;
+
+    // Allocate a option_state to which to add an option.
+    if (!option_state_allocate(&options, MDL)) {
+           atf_tc_fail("cannot allocat options state");
+    }
+
+    // Call add_option() to add the option to the option state.
+    if (!add_option(options, cid_code, cid_str, strlen(cid_str))) {
+           atf_tc_fail("add_option returned 0");
+    }
+
+    // Verify that calling add_option() only adds 1 to the option ref count.
+    if (option->refcnt != (refcnt_before + 1)) {
+        atf_tc_fail("after add_option(), count is wrong, before %d, after: %d",
+                    refcnt_before, option->refcnt);
+    }
+
+    // Derefrence the option_state, this should reduce the ref count to
+    // it's starting value.
+    option_state_dereference(&options, MDL);
+
+    // Verify that dereferencing option_state restores option ref count.
+    if (option->refcnt != refcnt_before) {
+        atf_tc_fail("after state deref, count is wrong, before %d, after: %d",
+                    refcnt_before, option->refcnt);
+    }
+}
+
 /* This macro defines main() method that will call specified
    test cases. tp and simple_test_case names can be whatever you want
    as long as it is a valid variable identifier. */
@@ -221,6 +274,7 @@ ATF_TP_ADD_TCS(tp)
     ATF_TP_ADD_TC(tp, option_refcnt);
     ATF_TP_ADD_TC(tp, pretty_print_option);
     ATF_TP_ADD_TC(tp, parse_X);
+    ATF_TP_ADD_TC(tp, add_option_ref_cnt);
 
     return (atf_no_error());
 }