[LDB][PATCH] Make LDB fail on invalid baseDN

Andrew Bartlett abartlet at samba.org
Sun Nov 11 21:40:15 GMT 2007


On Sun, 2007-11-11 at 10:07 -0500, simo wrote:
> 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?

I added such a caller.  See further in the patch. 

> 
> >         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 ?

I clarified the cases in the check below.  The if statement kept blowing
my mind every time I tried to parse it. 

> >         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.

Why should we do a full parse of the database entry, when we want just
to know the DN exists?

> 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.

I really don't see the value in doing that.

> > +
> > +               /* 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.

That code was the most difficult, hairy bit of logic I've had the
displeasure of trying to reverse-engineer.  I would rather not, thanks. 

> >         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) ?

That I'm quite happy to do. 

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Red Hat Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : http://lists.samba.org/archive/samba-technical/attachments/20071112/2e2fcfa8/attachment.bin


More information about the samba-technical mailing list