ldb: removing alloc from unpack_data

Making unpack flag LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC required
behaviour, since allocating data during unpack is slow and unnecessary
in all current usages. In any future unpack usage, if editing of
returned memory is required, some function that duplicates the message
should be used, such as one of the filter_attrs functions, or msg_copy.

Signed-off-by: Aaron Haslett <aaronhaslett@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>
Reviewed-by: Gary Lockyer <gary@catalyst.net.nz>

7 files changed, 52 insertions, 132 deletions

diff --git a/lib/ldb/common/ldb_pack.c b/lib/ldb/common/ldb_pack.c
index 448c577ae1b..60242e178f6 100644
--- a/lib/ldb/common/ldb_pack.c
+++ b/lib/ldb/common/ldb_pack.c
@@ -326,10 +326,8 @@ int ldb_unpack_data_only_attr_list_flags(struct ldb_context *ldb,
 	 * In typical use, most values are single-valued.  This makes
 	 * it quite expensive to allocate an array of ldb_val for each
 	 * of these, just to then hold the pointer to the data buffer
-	 * (in the LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC we don't
-	 * allocate the data).  So with
-	 * LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC we allocate this ahead
-	 * of time and use it for the single values where possible.
+	 * So with LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC we allocate this
+	 * ahead of time and use it for the single values where possible.
 	 * (This is used the the normal search case, but not in the
 	 * index case because of caller requirements).
 	 */
@@ -405,16 +403,7 @@ int ldb_unpack_data_only_attr_list_flags(struct ldb_context *ldb,
 			}
 		}
 		element = &message->elements[nelem];
-		if (flags & LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC) {
-			element->name = attr;
-		} else {
-			element->name = talloc_memdup(message->elements, attr, attr_len+1);
-
-			if (element->name == NULL) {
-				errno = ENOMEM;
-				goto failed;
-			}
-		}
+		element->name = attr;
 		element->flags = 0;
 
 		if (remaining < (attr_len + 1)) {
@@ -460,18 +449,7 @@ int ldb_unpack_data_only_attr_list_flags(struct ldb_context *ldb,
 			}
 
 			element->values[j].length = len;
-			if (flags & LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC) {
-				element->values[j].data = p + 4;
-			} else {
-				element->values[j].data = talloc_size(element->values, len+1);
-				if (element->values[j].data == NULL) {
-					errno = ENOMEM;
-					goto failed;
-				}
-				memcpy(element->values[j].data, p + 4,
-				       len);
-				element->values[j].data[len] = 0;
-			}
+			element->values[j].data = p + 4;
 			remaining -= len;
 			p += len+4+1;
 		}
diff --git a/lib/ldb/include/ldb_module.h b/lib/ldb/include/ldb_module.h
index b45142abe5c..bf4879d1ca5 100644
--- a/lib/ldb/include/ldb_module.h
+++ b/lib/ldb/include/ldb_module.h
@@ -528,20 +528,10 @@ int ldb_unpack_data(struct ldb_context *ldb,
 /*
  * Unpack a ldb message from a linear buffer in ldb_val
  *
- * Providing a list of attributes to this function allows selective unpacking.
- * Giving a NULL list (or a list_size of 0) unpacks all the attributes.
- *
- * Flags allow control of allocation, so that if
- * LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC is specified, then data in values are
- * not allocated, instead they point into the supplier constant buffer.
- *
  * If LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC is specified, then values
  * array are not allocated individually (for single-valued
  * attributes), instead they point into a single buffer per message.
  *
- * LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC is only valid when
- * LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC is also specified.
- *
  * Likewise if LDB_UNPACK_DATA_FLAG_NO_DN is specified, the DN is omitted.
  *
  * If LDB_UNPACK_DATA_FLAG_NO_ATTRS is specified, then no attributes
@@ -556,7 +546,7 @@ int ldb_unpack_data_only_attr_list_flags(struct ldb_context *ldb,
 					 unsigned int flags,
 					 unsigned int *nb_elements_in_db);
 
-#define LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC   0x0001
+/* currently unused (was NO_DATA_ALLOC)      0x0001 */
 #define LDB_UNPACK_DATA_FLAG_NO_DN           0x0002
 #define LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC 0x0004
 #define LDB_UNPACK_DATA_FLAG_NO_ATTRS        0x0008
diff --git a/lib/ldb/ldb_key_value/ldb_kv.c b/lib/ldb/ldb_key_value/ldb_kv.c
index c0846ba1b6a..53c326d36b2 100644
--- a/lib/ldb/ldb_key_value/ldb_kv.c
+++ b/lib/ldb/ldb_key_value/ldb_kv.c
@@ -644,8 +644,7 @@ static int ldb_kv_delete_internal(struct ldb_module *module, struct ldb_dn *dn)
 
 	/* in case any attribute of the message was indexed, we need
 	   to fetch the old record */
-	ret = ldb_kv_search_dn1(
-	    module, dn, msg, LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC);
+	ret = ldb_kv_search_dn1(module, dn, msg, 0);
 	if (ret != LDB_SUCCESS) {
 		/* not finding the old record is an error */
 		goto done;
@@ -902,8 +901,7 @@ int ldb_kv_modify_internal(struct ldb_module *module,
 		goto done;
 	}
 
-	ret = ldb_kv_search_dn1(
-	    module, msg->dn, msg2, LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC);
+	ret = ldb_kv_search_dn1(module, msg->dn, msg2, 0);
 	if (ret != LDB_SUCCESS) {
 		goto done;
 	}
@@ -1267,10 +1265,7 @@ static int ldb_kv_rename(struct ldb_kv_context *ctx)
 	}
 
 	/* we need to fetch the old record to re-add under the new name */
-	ret = ldb_kv_search_dn1(module,
-				req->op.rename.olddn,
-				msg,
-				LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC);
+	ret = ldb_kv_search_dn1(module, req->op.rename.olddn, msg, 0);
 	if (ret == LDB_ERR_INVALID_DN_SYNTAX) {
 		ldb_asprintf_errstring(ldb_module_get_ctx(module),
 				       "Invalid Old DN: %s",
diff --git a/lib/ldb/ldb_key_value/ldb_kv_cache.c b/lib/ldb/ldb_key_value/ldb_kv_cache.c
index 091d62d4158..b14697c2a5e 100644
--- a/lib/ldb/ldb_key_value/ldb_kv_cache.c
+++ b/lib/ldb/ldb_key_value/ldb_kv_cache.c
@@ -127,8 +127,7 @@ static int ldb_kv_attributes_load(struct ldb_module *module)
 	r = ldb_kv_search_dn1(module,
 			      dn,
 			      attrs_msg,
-			      LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC |
-				  LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC |
+			      LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC |
 				  LDB_UNPACK_DATA_FLAG_NO_DN);
 	talloc_free(dn);
 	if (r != LDB_SUCCESS && r != LDB_ERR_NO_SUCH_OBJECT) {
@@ -278,8 +277,7 @@ static int ldb_kv_index_load(struct ldb_module *module,
 	r = ldb_kv_search_dn1(module,
 			      indexlist_dn,
 			      ldb_kv->cache->indexlist,
-			      LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC |
-				  LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC |
+			      LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC |
 				  LDB_UNPACK_DATA_FLAG_NO_DN);
 	TALLOC_FREE(indexlist_dn);
 
diff --git a/lib/ldb/ldb_key_value/ldb_kv_index.c b/lib/ldb/ldb_key_value/ldb_kv_index.c
index 9f0de7d260e..0bedd2df517 100644
--- a/lib/ldb/ldb_key_value/ldb_kv_index.c
+++ b/lib/ldb/ldb_key_value/ldb_kv_index.c
@@ -405,7 +405,6 @@ normal_index:
 	ret = ldb_kv_search_dn1(module,
 				dn,
 				msg,
-				LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC |
 				LDB_UNPACK_DATA_FLAG_NO_DN |
 				/*
 				 * The entry point ldb_kv_search_indexed is
@@ -428,9 +427,8 @@ normal_index:
 
 	/*
 	 * we avoid copying the strings by stealing the list.  We have
-	 * to steal msg onto el->values (which looks odd) because we
-	 * asked for the memory to be allocated on msg, not on each
-	 * value with LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC above
+	 * to steal msg onto el->values (which looks odd) because
+	 * the memory is allocated on msg, not on each value.
 	 */
 	if (ldb_kv->cache->GUID_index_attribute == NULL) {
 		/* check indexing version number */
@@ -481,8 +479,7 @@ normal_index:
 		}
 
 		/*
-		 * The actual data is on msg, due to
-		 * LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC
+		 * The actual data is on msg.
 		 */
 		talloc_steal(list->dn, msg);
 		for (i = 0; i < list->count; i++) {
@@ -1675,7 +1672,6 @@ static int traverse_range_index(struct ldb_kv_private *ldb_kv,
 	ctx->error = ldb_unpack_data_only_attr_list_flags(ldb, &data,
 							  msg,
 							  NULL, 0,
-							  LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC |
 							  LDB_UNPACK_DATA_FLAG_NO_DN,
 							  NULL);
 
@@ -1694,9 +1690,8 @@ static int traverse_range_index(struct ldb_kv_private *ldb_kv,
 
 	/*
 	 * we avoid copying the strings by stealing the list.  We have
-	 * to steal msg onto el->values (which looks odd) because we
-	 * asked for the memory to be allocated on msg, not on each
-	 * value with LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC above
+	 * to steal msg onto el->values (which looks odd) because
+	 * the memory is allocated on msg, not on each value.
 	 */
 	if (version != LDB_KV_GUID_INDEXING_VERSION) {
 		/* This is quite likely during the DB startup
@@ -1758,8 +1753,7 @@ static int traverse_range_index(struct ldb_kv_private *ldb_kv,
 	}
 
 	/*
-	 * The actual data is on msg, due to
-	 * LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC
+	 * The actual data is on msg.
 	 */
 	talloc_steal(ctx->dn_list->dn, msg);
 	for (i = 0; i < additional_length; i++) {
@@ -2227,7 +2221,6 @@ static int ldb_kv_index_filter(struct ldb_kv_private *ldb_kv,
 				      ldb_kv,
 				      keys[i],
 				      msg,
-				      LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC |
 				      LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC |
 				      /*
 				       * The entry point ldb_kv_search_indexed is
@@ -3270,7 +3263,6 @@ static int re_key(struct ldb_kv_private *ldb_kv,
 	    (struct ldb_kv_reindex_context *)state;
 	struct ldb_module *module = ctx->module;
 	struct ldb_message *msg;
-	unsigned int nb_elements_in_db;
 	int ret;
 	struct ldb_val key2;
 	bool is_record;
@@ -3287,11 +3279,7 @@ static int re_key(struct ldb_kv_private *ldb_kv,
 		return -1;
 	}
 
-	ret = ldb_unpack_data_only_attr_list_flags(ldb, &val,
-						   msg,
-						   NULL, 0,
-						   LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC,
-						   &nb_elements_in_db);
+	ret = ldb_unpack_data(ldb, &val, msg);
 	if (ret != 0) {
 		ldb_debug(ldb, LDB_DEBUG_ERROR, "Invalid data for index %s\n",
 						ldb_dn_get_linearized(msg->dn));
@@ -3353,7 +3341,6 @@ static int re_index(struct ldb_kv_private *ldb_kv,
 	    (struct ldb_kv_reindex_context *)state;
 	struct ldb_module *module = ctx->module;
 	struct ldb_message *msg;
-	unsigned int nb_elements_in_db;
 	int ret;
 	bool is_record;
 
@@ -3369,11 +3356,7 @@ static int re_index(struct ldb_kv_private *ldb_kv,
 		return -1;
 	}
 
-	ret = ldb_unpack_data_only_attr_list_flags(ldb, &val,
-						   msg,
-						   NULL, 0,
-						   LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC,
-						   &nb_elements_in_db);
+	ret = ldb_unpack_data(ldb, &val, msg);
 	if (ret != 0) {
 		ldb_debug(ldb, LDB_DEBUG_ERROR, "Invalid data for index %s\n",
 						ldb_dn_get_linearized(msg->dn));
diff --git a/lib/ldb/ldb_key_value/ldb_kv_search.c b/lib/ldb/ldb_key_value/ldb_kv_search.c
index 7a6ae5668b0..2834279d86e 100644
--- a/lib/ldb/ldb_key_value/ldb_kv_search.c
+++ b/lib/ldb/ldb_key_value/ldb_kv_search.c
@@ -133,49 +133,45 @@ static int ldb_kv_parse_data_unpack(struct ldb_val key,
 				    void *private_data)
 {
 	struct ldb_kv_parse_data_unpack_ctx *ctx = private_data;
-	unsigned int nb_elements_in_db;
 	int ret;
 	struct ldb_context *ldb = ldb_module_get_ctx(ctx->module);
 	struct ldb_val data_parse = data;
 
 	struct ldb_kv_private *ldb_kv = ctx->ldb_kv;
 
-	if ((ctx->unpack_flags & LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC)) {
-		if ((ldb_kv->kv_ops->options & LDB_KV_OPTION_STABLE_READ_LOCK) &&
-		    (ctx->unpack_flags & LDB_UNPACK_DATA_FLAG_READ_LOCKED) &&
-		    !ldb_kv->kv_ops->transaction_active(ldb_kv)) {
-			/*
-			 * In the case where no transactions are active and
-			 * we're in a read-lock, we can point directly into
-			 * database memory.
-			 *
-			 * The database can't be changed underneath us and we
-			 * will duplicate this data in the call to filter.
-			 *
-			 * This is seen in:
-			 * - ldb_kv_index_filter
-			 * - ldb_kv_search_and_return_base
-			 */
-		} else {
-			/*
-			 * In every other case, if we got
-			 * LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC we need at least
-			 * do a memdup on the whole data buffer as that may
-			 * change later and the caller needs a stable result.
-			 *
-			 * During transactions, pointers could change and in
-			 * TDB, there just aren't the same guarantees.
-			 */
-			data_parse.data = talloc_memdup(ctx->msg,
-							data.data,
-							data.length);
-			if (data_parse.data == NULL) {
-				ldb_debug(ldb, LDB_DEBUG_ERROR,
-					  "Unable to allocate data(%d) for %*.*s\n",
-					  (int)data.length,
-					  (int)key.length, (int)key.length, key.data);
-				return LDB_ERR_OPERATIONS_ERROR;
-			}
+	if ((ldb_kv->kv_ops->options & LDB_KV_OPTION_STABLE_READ_LOCK) &&
+	    (ctx->unpack_flags & LDB_UNPACK_DATA_FLAG_READ_LOCKED) &&
+	    !ldb_kv->kv_ops->transaction_active(ldb_kv)) {
+		/*
+		 * In the case where no transactions are active and
+		 * we're in a read-lock, we can point directly into
+		 * database memory.
+		 *
+		 * The database can't be changed underneath us and we
+		 * will duplicate this data in the call to filter.
+		 *
+		 * This is seen in:
+		 * - ldb_kv_index_filter
+		 * - ldb_kv_search_and_return_base
+		 */
+	} else {
+		/*
+		 * In every other case, since unpack doesn't memdup, we need
+		 * to at least do a memdup on the whole data buffer as that
+		 * may change later and the caller needs a stable result.
+		 *
+		 * During transactions, pointers could change and in
+		 * TDB, there just aren't the same guarantees.
+		 */
+		data_parse.data = talloc_memdup(ctx->msg,
+						data.data,
+						data.length);
+		if (data_parse.data == NULL) {
+			ldb_debug(ldb, LDB_DEBUG_ERROR,
+				  "Unable to allocate data(%d) for %*.*s\n",
+				  (int)data.length,
+				  (int)key.length, (int)key.length, key.data);
+			return LDB_ERR_OPERATIONS_ERROR;
 		}
 	}
 
@@ -183,7 +179,7 @@ static int ldb_kv_parse_data_unpack(struct ldb_val key,
 						   ctx->msg,
 						   NULL, 0,
 						   ctx->unpack_flags,
-						   &nb_elements_in_db);
+						   NULL);
 	if (ret == -1) {
 		if (data_parse.data != data.data) {
 			talloc_free(data_parse.data);
@@ -514,7 +510,6 @@ static int search_func(struct ldb_kv_private *ldb_kv,
 	ret = ldb_unpack_data_only_attr_list_flags(ldb, &val,
 						   msg,
 						   NULL, 0,
-						   LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC|
 						   LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC,
 						   &nb_elements_in_db);
 	if (ret == -1) {
@@ -605,7 +600,6 @@ static int ldb_kv_search_and_return_base(struct ldb_kv_private *ldb_kv,
 	ret = ldb_kv_search_dn1(ctx->module,
 				ctx->base,
 				msg,
-				LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC |
 				LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC |
 				LDB_UNPACK_DATA_FLAG_READ_LOCKED);
 
diff --git a/source4/torture/ldb/ldb.c b/source4/torture/ldb/ldb.c
index dcbe919d5bf..261113841a5 100644
--- a/source4/torture/ldb/ldb.c
+++ b/source4/torture/ldb/ldb.c
@@ -1121,24 +1121,6 @@ static bool torture_ldb_unpack_flags(struct torture_context *torture)
 				 ldb_unpack_data_only_attr_list_flags(ldb, &data,
 								      msg,
 								      NULL, 0,
-								      LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC,
-								      &nb_elements_in_db),
-				 0,
-				 "ldb_unpack_data failed");
-
-	ldif.changetype = LDB_CHANGETYPE_NONE;
-	ldif.msg = msg;
-	ldif_text = ldb_ldif_write_string(ldb, mem_ctx, &ldif);
-
-	torture_assert_int_equal(torture,
-				 strcmp(ldif_text, dda1d01d_ldif), 0,
-				 "ldif form differs from binary form");
-
-	torture_assert_int_equal(torture,
-				 ldb_unpack_data_only_attr_list_flags(ldb, &data,
-								      msg,
-								      NULL, 0,
-								      LDB_UNPACK_DATA_FLAG_NO_DATA_ALLOC|
 								      LDB_UNPACK_DATA_FLAG_NO_VALUES_ALLOC,
 								      &nb_elements_in_db),
 				 0,