[LDB][PATCH] Make LDB fail on invalid baseDN
simo
simo at samba.org
Sun Nov 11 15:07:03 GMT 2007
On Fri, 2007-11-09 at 23:07 +1100, Andrew Bartlett wrote:
> Index: lib/ldb/ldb_tdb/ldb_search.c
> ===================================================================
> --- lib/ldb/ldb_tdb/ldb_search.c (revision 25905)
> +++ lib/ldb/ldb_tdb/ldb_search.c (working copy)
> @@ -214,8 +214,6 @@
> int ret;
> TDB_DATA tdb_key, tdb_data;
>
> - memset(msg, 0, sizeof(*msg));
> -
> /* form the key */
> tdb_key = ltdb_key(module, dn);
> if (!tdb_key.dptr) {
> @@ -227,6 +225,13 @@
> if (!tdb_data.dptr) {
> return LDB_ERR_NO_SUCH_OBJECT;
> }
> +
> + /* Perhaps the caller just wanted to check this exists? */
> + if (!msg) {
> + free(tdb_data.dptr);
> + return LDB_SUCCESS;
> + }
> + memset(msg, 0, sizeof(*msg));
What is this for ??
I see no caller that avoids passing msg, why you did this?
> msg->num_elements = 0;
> msg->elements = NULL;
> @@ -473,10 +478,6 @@
> struct ldb_reply *ares;
> int ret;
>
> - if ((( ! ldb_dn_is_valid(req->op.search.base)) ||
> ldb_dn_is_null(req->op.search.base)) &&
> - (req->op.search.scope == LDB_SCOPE_BASE ||
> req->op.search.scope == LDB_SCOPE_ONELEVEL))
> - return LDB_ERR_OPERATIONS_ERROR;
> -
Why you removed this check ??
It's a syntax check before any operation is done, what's the rationale ?
> if (ltdb_lock_read(module) != 0) {
> return LDB_ERR_OPERATIONS_ERROR;
> }
> @@ -496,6 +497,20 @@
> ltdb_unlock_read(module);
> return LDB_ERR_OPERATIONS_ERROR;
> }
> +
> + /* Ensure the DN actually exists, except for the subtree
> + * search over whole DB case permitted above */
> + if (ldb_dn_is_valid(req->op.search.base) &&
> (ldb_dn_is_null(req->op.search.base) == false)) {
> + ret = ltdb_search_dn1(module, req->op.search.base,
> NULL);
If this is the only reason to do the above change on msg == NULL, I'd
like you to not commit this patch. I don't think it is too difficult to
provide a msg structure here.
If this is just about saving cycles then consider that ltdb_serach_dn1()
is really simple and I'd rather see a special ltdb_check_base_dn()
function coded instead, so all other callers keep going with the
assumptions they had.
> +
> + /* These must have a valid DN */
> + } else if (req->op.search.scope == LDB_SCOPE_BASE ||
> req->op.search.scope == LDB_SCOPE_ONELEVEL) {
> + ltdb_unlock_read(module);
> + return LDB_ERR_INVALID_DN_SYNTAX;
> + } else {
> + ret = LDB_SUCCESS;
> + }
> +
I'd like you to leave the checks on top, and simply do the base DN as a
separate thing.
It keeps the code cleaner and simpler to read.
> ltdb_ac = talloc_get_type(req->handle->private_data, struct
> ltdb_context);
>
> ltdb_ac->tree = req->op.search.tree;
> @@ -503,12 +518,18 @@
> ltdb_ac->base = req->op.search.base;
> ltdb_ac->attrs = req->op.search.attrs;
>
> - ret = ltdb_search_indexed(req->handle);
> - if (ret == LDB_ERR_OPERATIONS_ERROR) {
> - ret = ltdb_search_full(req->handle);
> +
> + if (ret == LDB_SUCCESS) {
> + ret = ltdb_search_indexed(req->handle);
> + if (ret == LDB_ERR_OPERATIONS_ERROR) {
> + ret = ltdb_search_full(req->handle);
> + if (ret != LDB_SUCCESS) {
> + ldb_set_errstring(module->ldb,
> "Indexed and full searches both failed!\n");
> + }
> + }
> }
> +
> if (ret != LDB_SUCCESS) {
> - ldb_set_errstring(module->ldb, "Indexed and full
> searches both failed!\n");
> req->handle->state = LDB_ASYNC_DONE;
> req->handle->status = ret;
> }
Can you add an ldb_Set_errstring() for when the base DN is not found
here (or in ldb_search_dn1) ?
Simo.
--
Simo Sorce
Samba Team GPL Compliance Officer <simo at samba.org>
Senior Software Engineer at Red Hat Inc. <ssorce at redhat.com>
More information about the samba-technical
mailing list