[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