mgmtd: Prevent use after free

ci is picking up this use after free on occasion:

    ERROR: AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned: 0x6030001d94a0
        0 0x7fab994b7f04 in __interceptor_malloc_usable_size ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:119
        1 0x7fab994264f6 in __sanitizer::BufferedStackTrace::Unwind(unsigned long, unsigned long, void*, bool, unsigned int) ../../../../src/libsanitizer/sanitizer_common/sanitizer_stacktrace.h:131
        2 0x7fab994264f6 in __asan::asan_malloc_usable_size(void const*, unsigned long, unsigned long) ../../../../src/libsanitizer/asan/asan_allocator.cpp:1058
        3 0x7fab99039bcf in mt_count_free lib/memory.c:78
        4 0x7fab99039bcf in qfree lib/memory.c:130
        5 0x7fab98ff971a in hash_clean lib/hash.c:290
        6 0x56110cdb0e7f in mgmt_txn_hash_destroy mgmtd/mgmt_txn.c:1881
        7 0x56110cdb0e7f in mgmt_txn_destroy mgmtd/mgmt_txn.c:2013
        8 0x56110cd8e5de in mgmt_terminate mgmtd/mgmt.c:91
        9 0x56110cd8e003 in sigint mgmtd/mgmt_main.c:90
        10 0x7fab990bf4b0 in frr_sigevent_process lib/sigevent.c:117
        11 0x7fab990ea7a1 in event_fetch lib/event.c:1740
        12 0x7fab9901a24e in frr_run lib/libfrr.c:1245
        13 0x56110cd8e21f in main mgmtd/mgmt_main.c:290
        14 0x7fab98af9249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
        15 0x7fab98af9304 in __libc_start_main_impl ../csu/libc-start.c:360
        16 0x56110cd8dd30 in _start (/usr/lib/frr/mgmtd+0x3ad30)

    0x6030001d94a0 is located 0 bytes inside of 24-byte region [0x6030001d94a0,0x6030001d94b8)
    freed by thread T0 here:
        0 0x7fab994b76a8 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52
        1 0x7fab99039bf0 in qfree lib/memory.c:131
        2 0x7fab98ff93e1 in hash_release lib/hash.c:227
        3 0x56110cdaabdc in mgmt_txn_unlock mgmtd/mgmt_txn.c:1931
        4 0x56110cdab049 in mgmt_txn_delete mgmtd/mgmt_txn.c:1841
        5 0x56110cdab0ce in mgmt_txn_hash_free mgmtd/mgmt_txn.c:1864
        6 0x7fab98ff970b in hash_clean lib/hash.c:288
        7 0x56110cdb0e7f in mgmt_txn_hash_destroy mgmtd/mgmt_txn.c:1881
        8 0x56110cdb0e7f in mgmt_txn_destroy mgmtd/mgmt_txn.c:2013
        9 0x56110cd8e5de in mgmt_terminate mgmtd/mgmt.c:91
        10 0x56110cd8e003 in sigint mgmtd/mgmt_main.c:90
        11 0x7fab990bf4b0 in frr_sigevent_process lib/sigevent.c:117
        12 0x7fab990ea7a1 in event_fetch lib/event.c:1740
        13 0x7fab9901a24e in frr_run lib/libfrr.c:1245
        14 0x56110cd8e21f in main mgmtd/mgmt_main.c:290
        15 0x7fab98af9249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

    previously allocated by thread T0 here:
        0 0x7fab994b83b7 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77
        1 0x7fab990392fd in qcalloc lib/memory.c:106
        2 0x7fab98ff8b4f in hash_get lib/hash.c:156
        3 0x56110cdb13ae in mgmt_txn_create_new mgmtd/mgmt_txn.c:1825
        4 0x56110cdb3b4d in mgmt_txn_notify_be_adapter_conn mgmtd/mgmt_txn.c:2212
        5 0x56110cd91178 in mgmt_be_adapter_conn_init mgmtd/mgmt_be_adapter.c:842
        6 0x7fab990ec6de in event_call lib/event.c:2019
        7 0x7fab9901a243 in frr_run lib/libfrr.c:1246
        8 0x56110cd8e21f in main mgmtd/mgmt_main.c:290
        9 0x7fab98af9249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58

The only time that mgmt_txn_hash_free is called is in hash_clean.
There are other places that mgmt_txn_unlock/delete are called and
hash_release should be called.  Let's just notice when mgmtd is
being called from the hash_clean and not call hash_release (since
we know it is being released already)

Signed-off-by: Donald Sharp <sharpd@nvidia.com>

diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c
index 4afab389b8d553d8af3f89ba1cc35f4a6c3e511f..483dfab8e85e005a1291ec5e596d0e2b81915894 100644 (file)
--- a/mgmtd/mgmt_txn.c
+++ b/mgmtd/mgmt_txn.c
@@ -22,7 +22,7 @@
 #define __log_err(fmt, ...) zlog_err("%s: ERROR: " fmt, __func__, ##__VA_ARGS__)
 
 #define MGMTD_TXN_LOCK(txn)   mgmt_txn_lock(txn, __FILE__, __LINE__)
-#define MGMTD_TXN_UNLOCK(txn) mgmt_txn_unlock(txn, __FILE__, __LINE__)
+#define MGMTD_TXN_UNLOCK(txn, in_hash_free) mgmt_txn_unlock(txn, in_hash_free, __FILE__, __LINE__)
 
 enum mgmt_txn_event {
        MGMTD_TXN_PROC_SETCFG = 1,
@@ -295,7 +295,7 @@ static inline const char *mgmt_txn_commit_phase_str(struct mgmt_txn_ctx *txn)
 }
 
 static void mgmt_txn_lock(struct mgmt_txn_ctx *txn, const char *file, int line);
-static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, const char *file,
+static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, bool in_hash_free, const char *file,
                            int line);
 static int mgmt_txn_send_be_txn_delete(struct mgmt_txn_ctx *txn,
                                       struct mgmt_be_client_adapter *adapter);
@@ -357,7 +357,7 @@ static void mgmt_txn_cfg_batch_free(struct mgmt_txn_be_cfg_batch **batch)
                }
        }
 
-       MGMTD_TXN_UNLOCK(&(*batch)->txn);
+       MGMTD_TXN_UNLOCK(&(*batch)->txn, false);
 
        XFREE(MTYPE_MGMTD_TXN_CFG_BATCH, *batch);
        *batch = NULL;
@@ -552,7 +552,7 @@ static void mgmt_txn_req_free(struct mgmt_txn_req **txn_req)
                      (*txn_req)->req_id, mgmt_txn_reqs_count(req_list));
        }
 
-       MGMTD_TXN_UNLOCK(&(*txn_req)->txn);
+       MGMTD_TXN_UNLOCK(&(*txn_req)->txn, false);
        XFREE(MTYPE_MGMTD_TXN_REQ, (*txn_req));
        *txn_req = NULL;
 }
@@ -1836,9 +1836,9 @@ static struct mgmt_txn_ctx *mgmt_txn_create_new(uint64_t session_id,
        return txn;
 }
 
-static void mgmt_txn_delete(struct mgmt_txn_ctx **txn)
+static void mgmt_txn_delete(struct mgmt_txn_ctx **txn, bool in_hash_free)
 {
-       MGMTD_TXN_UNLOCK(txn);
+       MGMTD_TXN_UNLOCK(txn, in_hash_free);
 }
 
 static unsigned int mgmt_txn_hash_key(const void *data)
@@ -1861,7 +1861,7 @@ static void mgmt_txn_hash_free(void *data)
 {
        struct mgmt_txn_ctx *txn = data;
 
-       mgmt_txn_delete(&txn);
+       mgmt_txn_delete(&txn, true);
 }
 
 static void mgmt_txn_hash_init(void)
@@ -1911,8 +1911,7 @@ static void mgmt_txn_lock(struct mgmt_txn_ctx *txn, const char *file, int line)
              mgmt_txn_type2str(txn->type), txn->txn_id, txn->refcount);
 }
 
-static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, const char *file,
-                           int line)
+static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, bool in_hash_free, const char *file, int line)
 {
        assert(*txn && (*txn)->refcount);
 
@@ -1928,7 +1927,9 @@ static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, const char *file,
                EVENT_OFF((*txn)->proc_comm_cfg);
                EVENT_OFF((*txn)->comm_cfg_timeout);
                EVENT_OFF((*txn)->get_tree_timeout);
-               hash_release(mgmt_txn_mm->txn_hash, *txn);
+               if (!in_hash_free)
+                       hash_release(mgmt_txn_mm->txn_hash, *txn);
+
                mgmt_txns_del(&mgmt_txn_mm->txn_list, *txn);
 
                __dbg("Deleted %s txn-id: %" PRIu64 " session-id: %" PRIu64,
@@ -1945,7 +1946,7 @@ static void mgmt_txn_cleanup_txn(struct mgmt_txn_ctx **txn)
 {
        /* TODO: Any other cleanup applicable */
 
-       mgmt_txn_delete(txn);
+       mgmt_txn_delete(txn, false);
 }
 
 static void mgmt_txn_cleanup_all_txns(void)
@@ -2037,7 +2038,7 @@ void mgmt_destroy_txn(uint64_t *txn_id)
        if (!txn)
                return;
 
-       mgmt_txn_delete(&txn);
+       mgmt_txn_delete(&txn, false);
        *txn_id = MGMTD_TXN_ID_NONE;
 }