From 535debb312b8dc5a9de6b6fe2543af84e9534a23 Mon Sep 17 00:00:00 2001 From: Andrew Bartlett Date: Thu, 27 Feb 2020 11:30:00 +1300 Subject: ldb: Add mem_ctx argument to ldb_kv_index_key() This avoids using "ldb" as the memory context in most cases, and may avoid a long-term memory leak if future changes cause dn_key not to be freed. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14299 Signed-off-by: Andrew Bartlett Reviewed-by: Gary Lockyer --- lib/ldb/ldb_key_value/ldb_kv_index.c | 58 ++++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 15 deletions(-) diff --git a/lib/ldb/ldb_key_value/ldb_kv_index.c b/lib/ldb/ldb_key_value/ldb_kv_index.c index 2935b95c4dd..5a24b074e1c 100644 --- a/lib/ldb/ldb_key_value/ldb_kv_index.c +++ b/lib/ldb/ldb_key_value/ldb_kv_index.c @@ -975,6 +975,7 @@ int ldb_kv_index_transaction_cancel(struct ldb_module *module) the caller is responsible for freeing */ static struct ldb_dn *ldb_kv_index_key(struct ldb_context *ldb, + TALLOC_CTX *mem_ctx, struct ldb_kv_private *ldb_kv, const char *attr, const struct ldb_val *value, @@ -1110,7 +1111,7 @@ static struct ldb_dn *ldb_kv_index_key(struct ldb_context *ldb, if (should_b64_encode) { size_t vstr_len = 0; - char *vstr = ldb_base64_encode(ldb, (char *)v.data, v.length); + char *vstr = ldb_base64_encode(mem_ctx, (char *)v.data, v.length); if (!vstr) { talloc_free(attr_folded); return NULL; @@ -1131,7 +1132,7 @@ static struct ldb_dn *ldb_kv_index_key(struct ldb_context *ldb, * Note: the double hash "##" is not a typo and * indicates that the following value is base64 encoded */ - ret = ldb_dn_new_fmt(ldb, ldb, "%s#%s##%.*s", + ret = ldb_dn_new_fmt(mem_ctx, ldb, "%s#%s##%.*s", LDB_KV_INDEX, attr_for_dn, frmt_len, vstr); } else { @@ -1141,7 +1142,7 @@ static struct ldb_dn *ldb_kv_index_key(struct ldb_context *ldb, * Note: the double colon "::" is not a typo and * indicates that the following value is base64 encoded */ - ret = ldb_dn_new_fmt(ldb, ldb, "%s:%s::%.*s", + ret = ldb_dn_new_fmt(mem_ctx, ldb, "%s:%s::%.*s", LDB_KV_INDEX, attr_for_dn, frmt_len, vstr); } @@ -1163,13 +1164,13 @@ static struct ldb_dn *ldb_kv_index_key(struct ldb_context *ldb, * Truncated keys are placed in a separate key space * from the non truncated keys */ - ret = ldb_dn_new_fmt(ldb, ldb, "%s#%s#%.*s", + ret = ldb_dn_new_fmt(mem_ctx, ldb, "%s#%s#%.*s", LDB_KV_INDEX, attr_for_dn, frmt_len, (char *)v.data); } else { frmt_len = v.length; *truncation = KEY_NOT_TRUNCATED; - ret = ldb_dn_new_fmt(ldb, ldb, "%s:%s:%.*s", + ret = ldb_dn_new_fmt(mem_ctx, ldb, "%s:%s:%.*s", LDB_KV_INDEX, attr_for_dn, frmt_len, (char *)v.data); } @@ -1269,9 +1270,15 @@ static int ldb_kv_index_dn_simple(struct ldb_module *module, return LDB_ERR_OPERATIONS_ERROR; } - /* the attribute is indexed. Pull the list of DNs that match the - search criterion */ + /* + * the attribute is indexed. Pull the list of DNs that match the + * search criterion + * + * list is used as a memory context as it has a shorter life + * than 'ldb'. Regardless we talloc_free() 'dn' below. + */ dn = ldb_kv_index_key(ldb, + list, ldb_kv, tree->u.equality.attr, &tree->u.equality.value, @@ -1957,7 +1964,7 @@ static int ldb_kv_index_dn_ordered(struct ldb_module *module, return ldb_module_oom(module); } - key_dn = ldb_kv_index_key(ldb, ldb_kv, tree->u.comparison.attr, + key_dn = ldb_kv_index_key(ldb, tmp_ctx, ldb_kv, tree->u.comparison.attr, &tree->u.comparison.value, NULL, &truncation); if (!key_dn) { @@ -1978,7 +1985,8 @@ static int ldb_kv_index_dn_ordered(struct ldb_module *module, return LDB_ERR_OPERATIONS_ERROR; } - key_dn = ldb_kv_index_key(ldb, ldb_kv, tree->u.comparison.attr, + key_dn = ldb_kv_index_key(ldb, tmp_ctx, + ldb_kv, tree->u.comparison.attr, NULL, NULL, &truncation); if (!key_dn) { TALLOC_FREE(tmp_ctx); @@ -2098,7 +2106,13 @@ static int ldb_kv_index_dn_attr(struct ldb_module *module, return LDB_ERR_OPERATIONS_ERROR; } val.length = strlen((char *)val.data); - key = ldb_kv_index_key(ldb, ldb_kv, attr, &val, NULL, truncation); + + /* + * We use list as a TALLOC_CTX to provide a shorter-lived + * memory context than ldb, even as the result is freed with + * the talloc_free(key) below. + */ + key = ldb_kv_index_key(ldb, list, ldb_kv, attr, &val, NULL, truncation); if (!key) { ldb_oom(ldb); return LDB_ERR_OPERATIONS_ERROR; @@ -2670,8 +2684,13 @@ static int ldb_kv_index_add1(struct ldb_module *module, return LDB_ERR_OPERATIONS_ERROR; } - dn_key = ldb_kv_index_key( - ldb, ldb_kv, el->name, &el->values[v_idx], &a, &truncation); + dn_key = ldb_kv_index_key(ldb, + list, + ldb_kv, + el->name, + &el->values[v_idx], + &a, + &truncation); if (!dn_key) { talloc_free(list); return LDB_ERR_OPERATIONS_ERROR; @@ -2695,7 +2714,6 @@ static int ldb_kv_index_add1(struct ldb_module *module, talloc_free(list); return LDB_ERR_CONSTRAINT_VIOLATION; } - talloc_steal(list, dn_key); ret = ldb_kv_dn_list_load(module, ldb_kv, dn_key, list, DN_LIST_MUTABLE); @@ -3195,8 +3213,18 @@ int ldb_kv_index_del_value(struct ldb_module *module, return LDB_SUCCESS; } - dn_key = ldb_kv_index_key( - ldb, ldb_kv, el->name, &el->values[v_idx], NULL, &truncation); + /* + * ldb is being used as the memory context to ldb_kv_index_key + * as dn_key itself is also used as the TALLOC_CTX for the + * rest of this function. + */ + dn_key = ldb_kv_index_key(ldb, + ldb, + ldb_kv, + el->name, + &el->values[v_idx], + NULL, + &truncation); /* * We ignore key truncation in ltdb_index_add1() so * match that by ignoring it here as well -- cgit v1.2.1