Releases, locking and ldb

[Andrew Bartlett]

On Tue, 2017-06-27 at 21:46 +1200, Andrew Bartlett via samba-technical
wrote:
> On Tue, 2017-06-27 at 08:24 +0200, Stefan Metzmacher via samba-
> technical wrote:
> > 
> > The good thing is that due to LDB_MODULE_CHECK_VERSION()
> > Samba and sssd are only runtime compatible with the version
> > they're build against.
> > 
> > Would "ldb:tdb: Ensure we correctly decrement ltdb->read_lock_count"
> > affect any old caller in any bad way?
> 
> I presume you mean some other hypothetical application beyond Samba and
> SSSD?
> 
> The primary change is that during the ldb callbacks, the DB is locked,
> rather than unlocked.  It is no longer possible to start a transaction
> (write operation) during an ldb callback.  Instead, as good practice
> would already dictate, you must start the transaction before the
> search. 
> 
> > And what about "tdb: Remove locking from tdb_traverse_read()"?
> 
> I'm not sure about this one.  As we can't have one without the other I
> don't know (and haven't studied) exactly what else it might impact. 
> 
> I don't think this without the ldb patches is actively harmful however.
> 
> > Whould something like this on the samba_dsdb module
> > in 4.6 work around the problems (and keep it only as broken as it
> > already is):
> > - always call ldb_handle_use_global_event_context()
> 
> I don't think this case is worth worrying about.  It is rootdse
> modified over LDAP in the single process model (only). 
> 
> > - overwrite read_lock/read_unlock() to be a no-op
> 
> No, this gets us to the point I was at during SambaXP, with random
> LDB_ERR_BUSY / 'failed to upgrade hash locks', as we won't have a
> global lock order.
> 
> I don't think it makes any difference to actually to have it pass the
> read_lock()/read_unlock() down to sam.ldb.
> 
> I personally think we should just declare the incomparability, ideally
> at build time, and move on.  (I tried to create an #ifdef hack to
> detect this at build time, but wasn't able to build a reliable one
> yet). 
> 
> I realise that everything that can happen will happen, but still: how
> likely are they to be mixed in the real world?

I guess more likely than I care to admit.  Try the attached for a
compile-time way to prevent this.

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba
-------------- next part --------------
diff --git a/lib/ldb/include/ldb_module.h b/lib/ldb/include/ldb_module.h
index 3d56e68..7140e18 100644
--- a/lib/ldb/include/ldb_module.h
+++ b/lib/ldb/include/ldb_module.h
@@ -33,6 +33,13 @@
 #ifndef _LDB_MODULE_H_
 #define _LDB_MODULE_H_
 
+#if !defined(SAMBA_PATCHED_FOR_LDB_LOCKING) && \
+	defined(_SAMBA_BUILD_)
+#if (_SAMBA_BUILD_ == 4)
+#error "Samba < 4.7 is not compatible with this version of ldb due to assumptions around read locks"
+#endif
+#endif
+
 #include <ldb.h>
 
 struct ldb_context;
diff --git a/lib/ldb/wscript b/lib/ldb/wscript
index 1fce3b3..dfdaba4 100644
--- a/lib/ldb/wscript
+++ b/lib/ldb/wscript
@@ -105,6 +105,8 @@ def configure(conf):
 
     conf.DEFINE('HAVE_CONFIG_H', 1, add_to_cflags=True)
 
+    conf.DEFINE('SAMBA_PATCHED_FOR_LDB_LOCKING', 1, add_to_cflags=True)
+    
     conf.SAMBA_CONFIG_H()
 
     conf.SAMBA_CHECK_UNDEFINED_SYMBOL_FLAGS()