dsdb: Be careful to avoid use of the expensive talloc_is_parent()

The wrong talloc API was selected while addressing a memory leak.

commit ee2fe56ba0ef6626b634376e8dc2185aa89f8c99
Author: Aaron Haslett <aaronhaslett@catalyst.net.nz>
Date:   Tue Nov 27 11:07:44 2018 +1300

    drepl: memory leak fix

    Fixes a memory leak where schema reference attached to ldb
    instance is lost before it can be freed.

    BUG: https://bugzilla.samba.org/show_bug.cgi?id=14042

    Signed-off-by: Aaron Haslett <aaronhaslett@catalyst.net.nz>
    Reviewed-by: Andrew Bartlett <abartlet@samba.org>
    Reviewed-by: Garming Sam <garming@catalyst.net.nz>
    Autobuild-User(master): Garming Sam <garming@samba.org>
    Autobuild-Date(master): Wed Jul 17 06:17:10 UTC 2019 on sn-devel-184

By using talloc_get_parent() walking the entire talloc tree is
avoided.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=14806

Signed-off-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>

diff --git a/source4/dsdb/schema/schema_set.c b/source4/dsdb/schema/schema_set.c
index f4934917c7c32940dc5bf7435a8766df550f4b48..45faa0912ec648c59b8b93b3909c87c00e18c6ae 100644 (file)
--- a/source4/dsdb/schema/schema_set.c
+++ b/source4/dsdb/schema/schema_set.c
@@ -698,6 +698,8 @@ int dsdb_reference_schema(struct ldb_context *ldb, struct dsdb_schema *schema,
 {
        int ret;
        void *ptr;
+       void *schema_parent = NULL;
+       bool is_already_parent;
        struct dsdb_schema *old_schema;
        old_schema = ldb_get_opaque(ldb, "dsdb_schema");
        ret = ldb_set_opaque(ldb, "dsdb_schema", schema);
@@ -710,8 +712,9 @@ int dsdb_reference_schema(struct ldb_context *ldb, struct dsdb_schema *schema,
        talloc_unlink(ldb, old_schema);
 
        /* Reference schema on ldb if it wasn't done already */
-       ret = talloc_is_parent(ldb, schema);
-       if (ret == 0) {
+       schema_parent = talloc_parent(schema);
+       is_already_parent = (schema_parent == ldb);
+       if (!is_already_parent) {
                ptr = talloc_reference(ldb, schema);
                if (ptr == NULL) {
                        return ldb_oom(ldb);
@@ -775,10 +778,10 @@ int dsdb_set_global_schema(struct ldb_context *ldb)
        /* Don't write indices and attributes, it's expensive */
        ret = dsdb_schema_set_indices_and_attributes(ldb, global_schema, SCHEMA_MEMORY_ONLY);
        if (ret == LDB_SUCCESS) {
-               /* If ldb doesn't have a reference to the schema, make one,
-                * just in case the original copy is replaced */
-               ret = talloc_is_parent(ldb, global_schema);
-               if (ret == 0) {
+               void *schema_parent = talloc_parent(global_schema);
+               bool is_already_parent =
+                       (schema_parent == ldb);
+               if (!is_already_parent) {
                        ptr = talloc_reference(ldb, global_schema);
                        if (ptr == NULL) {
                                return ldb_oom(ldb);
@@ -809,7 +812,6 @@ struct dsdb_schema *dsdb_get_schema(struct ldb_context *ldb, TALLOC_CTX *referen
        dsdb_schema_refresh_fn refresh_fn;
        struct ldb_module *loaded_from_module;
        bool use_global_schema;
-       int ret;
        TALLOC_CTX *tmp_ctx = talloc_new(reference_ctx);
        if (tmp_ctx == NULL) {
                return NULL;
@@ -860,13 +862,28 @@ struct dsdb_schema *dsdb_get_schema(struct ldb_context *ldb, TALLOC_CTX *referen
        /* This removes the extra reference above */
        talloc_free(tmp_ctx);
 
-       /* If ref ctx exists and doesn't already reference schema, then add
-        * a reference.  Otherwise, just return schema.*/
-       ret = talloc_is_parent(reference_ctx, schema_out);
-       if ((ret == 1) || (!reference_ctx)) {
+       /*
+        * If ref ctx exists and doesn't already reference schema, then add
+        * a reference.  Otherwise, just return schema.
+        *
+        * We must use talloc_parent(), which is not quite free (there
+        * is no direct parent pointer in talloc, only one on the
+        * first child within a linked list), but is much cheaper than
+        * talloc_is_parent() which walks the whole tree up to the top
+        * looking for a potential grand-grand(etc)-parent.
+        */
+       if (reference_ctx == NULL) {
                return schema_out;
        } else {
-               return talloc_reference(reference_ctx, schema_out);
+               void *schema_parent = talloc_parent(schema_out);
+               bool is_already_parent =
+                       schema_parent == reference_ctx;
+               if (is_already_parent) {
+                       return schema_out;
+               } else {
+                       return talloc_reference(reference_ctx,
+                                               schema_out);
+               }
        }
 }