[PATCH] tdb traverse transaction unlocking bug
Rusty Russell
rusty at rustcorp.com.au
Fri Jul 17 23:11:41 MDT 2009
On Sat, 18 Jul 2009 08:44:49 am Michael Adam wrote:
> Hi Rusty,
>
> thanks for bringing this up (again)!
Yes, unfortunately I only found Volker's fix for the samba tdb after I found
both problems...
> > @@ -319,15 +323,13 @@ int tdb_transaction_lock(struct tdb_context *tdb,
> > int ltype) */
> > int tdb_transaction_unlock(struct tdb_context *tdb)
> > {
> > - int ret;
> > - if (!tdb->have_transaction_lock) {
> > + if (tdb->global_lock.count) {
> > return 0;
> > }
>
> Hmm, I don't quite get why you remove the check
> if (!tdb->have_transaction_lock):
Ah, I moved it down. The patch didn't make it clear, but it's now:
int tdb_transaction_unlock(struct tdb_context *tdb)
{
if (tdb->global_lock.count) {
return 0;
}
if (--tdb->have_transaction_lock) {
return 0;
}
return tdb->methods->tdb_brlock(tdb, TRANSACTION_LOCK, F_UNLCK,
F_SETLKW, 0, 1);
}
(I felt that clearer than doing the dec in one line...)
> Either you rely on the caller to only call transaction_unlock()
> when transaction_lock() has been called previously, and then
> you don't need to check for tdb->global_lock.count either,
> or else you should also check for tdb->have_transaction_lock
> or you risk decrementing have_transaction_lock when it is == 0.
You're right, the current code doesn't assume the caller is correct; I should
restore this behavior, like so:
if (tdb->have_transaction_lock > 0) {
tdb->have_transaction_lock--;
return 0;
}
ret = tdb->methods->....
if (ret == 0) {
tdb->have_transaction_lock = 0;
}
return ret;
> > - bool have_transaction_lock;
> > + int have_transaction_lock;
> > volatile sig_atomic_t *interrupt_sig_ptr;
> > };
>
> Maybe we should also rename have_transaction_lock to
> transaction_lock_count (or so).
Hmm, traverse_read & traverse_write were my model, but I agree it's much
clearer.
I'll create a new patch.
Thanks!
Rusty.
More information about the samba-technical
mailing list