[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