[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