search

389ds / 389-ds-base

The enterprise-class Open Source LDAP server for Linux
https://www.port389.org/
Other
211 stars 93 forks source link

Memory leak in ndn_cache_lookup #4637

Closed vashirov closed 3 years ago

vashirov commented 3 years ago

Any test reports leak in ndn_cache_lookup ldap/servers/slapd/dn.c:3154 (41697 occurrences)

=================================================================
==287==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1518 byte(s) in 21 object(s) allocated from:
    #0 0x7f525e27d547 in strdup (/lib64/libasan.so.6+0x58547)
    #1 0x7f525dd35504 in slapi_ch_strdup (/usr/lib64/dirsrv/libslapd.so.0+0x1a4504)
    #2 0x7f525dd40d13 in ndn_cache_lookup ldap/servers/slapd/dn.c:3154
    #3 0x7f525dd40d13 in slapi_dn_normalize_ext ldap/servers/slapd/dn.c:587
    #4 0x7f525dd42bff in slapi_create_dn_string (/usr/lib64/dirsrv/libslapd.so.0+0x1b1bff)
    #5 0x7f52596945b9 in ldbm_instance_config_add_index_entry ldap/servers/slapd/back-ldbm/ldbm_index_config.c:325
    #6 0x7f525969490b in ldbm_instance_create_default_user_indexes ldap/servers/slapd/back-ldbm/ldbm_index_config.c:474
    #7 0x7f5259696d52 in ldbm_instance_add_instance_entry_callback ldap/servers/slapd/back-ldbm/ldbm_instance_config.c:1093
    #8 0x7f52596813a1 in ldbm_config_read_instance_entries ldap/servers/slapd/back-ldbm/ldbm_config.c:1014
    #9 0x7f5259681579 in ldbm_config_load_dse_info ldap/servers/slapd/back-ldbm/ldbm_config.c:1105
    #10 0x7f525966e51d in dblayer_setup ldap/servers/slapd/back-ldbm/dblayer.c:193
    #11 0x7f52596a861d in ldbm_back_start ldap/servers/slapd/back-ldbm/start.c:46
    #12 0x7f525dd904bb in plugin_call_func ldap/servers/slapd/plugin.c:2002
    #13 0x7f525dd9230c in plugin_call_one ldap/servers/slapd/plugin.c:1951
    #14 0x7f525dd9230c in plugin_dependency_startall ldap/servers/slapd/plugin.c:1705
    #15 0x56069f6cd6e7 in main (/usr/sbin/ns-slapd+0xe66e7)
    #16 0x7f525d6571e1 in __libc_start_main (/lib64/libc.so.6+0x281e1)
    #17 0x56069f6d136d in _start (/usr/sbin/ns-slapd+0xea36d)

Another occurrence in dirsrvtests/tests/suites/syncrepl_plugin/basic_test.py::test_sync_repl_cookie_add_del

Indirect leak of 18 byte(s) in 1 object(s) allocated from:
    #0 0x7f500f599547 in strdup (/lib64/libasan.so.6+0x58547)
    #1 0x7f500f051504 in slapi_ch_strdup (/usr/lib64/dirsrv/libslapd.so.0+0x1a4504)
    #2 0x7f500f05cce3 in ndn_cache_lookup ldap/servers/slapd/dn.c:3143
    #3 0x7f500f05cce3 in slapi_dn_normalize_ext ldap/servers/slapd/dn.c:587
    #4 0x7f500f05f631 in slapi_sdn_get_dn (/usr/lib64/dirsrv/libslapd.so.0+0x1b2631)
    #5 0x7f500f05f877 in slapi_sdn_get_ndn (/usr/lib64/dirsrv/libslapd.so.0+0x1b2877)
    #6 0x7f500f05f971 in slapi_sdn_issuffix (/usr/lib64/dirsrv/libslapd.so.0+0x1b2971)
    #7 0x7f500f0606dc in slapi_sdn_scope_test (/usr/lib64/dirsrv/libslapd.so.0+0x1b36dc)
    #8 0x7f500aa595ca in sync_queue_change ldap/servers/plugins/sync/sync_persist.c:473
    #9 0x7f500aa59b34 in sync_update_persist_op ldap/servers/plugins/sync/sync_persist.c:336
    #10 0x7f500aa59e8c in sync_add_persist_post_op ldap/servers/plugins/sync/sync_persist.c:358
    #11 0x7f500f0ac4bb in plugin_call_func ldap/servers/slapd/plugin.c:2002
    #12 0x7f500f0ac6d3 in plugin_call_list ldap/servers/slapd/plugin.c:1945
    #13 0x7f5007e0fdf4 in ldbm_back_add ldap/servers/slapd/back-ldbm/ldbm_add.c:1413
    #14 0x7f500f052e1e in op_shared_add ldap/servers/slapd/add.c:758
    #15 0x7f500f053e64 in do_add (/usr/lib64/dirsrv/libslapd.so.0+0x1a6e64)
    #16 0x55e92cb02f65 in connection_dispatch_operation ldap/servers/slapd/connection.c:620
    #17 0x55e92cb02f65 in connection_threadmain ldap/servers/slapd/connection.c:1777
    #18 0x7f500e72114f  (/lib64/libnspr4.so+0x2b14f)
    #19 0x7f500e6b53f8 in start_thread (/lib64/libpthread.so.0+0x93f8)
    #20 0x7f500ea4cb52 in __GI___clone (/lib64/libc.so.6+0x101b52)
Firstyear commented 3 years ago

I'll have a look at this this morning @vashirov

Firstyear commented 3 years ago

Looking at the code, the logic hasn't changed between the C / Rust ndn cache:

// Rust 
    const char *cache_ndn = cache_char_read_get(read_txn, dn);
    if (cache_ndn != NULL) {
        *ndn = slapi_ch_strdup(cache_ndn);
        /*
         * We have to complete the read after the strdup else it's
         * not safe to access the pointer.
         */
        cache_char_read_complete(read_txn);
        *rc = 1;
        return 1;
    } else {
        cache_char_read_complete(read_txn);
        /* If we miss, we need to duplicate dn to udn here. */
        *udn = slapi_ch_strdup(dn);
        *rc = 0;
        return 0;
    }

// C

                /* Update that we have a hit.*/
                *ndn = slapi_ch_strdup(node->ndn);
                /* Indicate to the caller to free this. */
                *rc = 1;
                ndn_thread_cache_commit_status(t_cache);
                return 1;

...

    /* If we miss, we need to duplicate dn to udn here. */
    *udn = slapi_ch_strdup(dn);
    *rc = 0;
    ndn_thread_cache_commit_status(t_cache);
    return 0;

Does this leak occur when you use --disable-rust? I'm wondering if it's caused by a higher level ....

vashirov commented 3 years ago

This leak happens only when built with Rust.

vashirov commented 3 years ago

I've also ran some tests with ThreadSanitizer. It reported data races around ndn_cache_lookup and concread. tsan.tar.gz

mreynolds389 commented 3 years ago

I see a ton of memory leaks in NDN as well. Now I can not seem to find the NDN Rust code, so I am not sure how it frees the hash tables on shutdown, but it looks NDN has a per thread cache, but it does not take into account the main() thread (only worker threads)! So somewhere in main() we need to free its NDN cache/hash tables, but I don't know how to do that with the Rust plugin.

Firstyear commented 3 years ago

@mreynolds389 I can have a look at it later, I've just been a bit slow lately.