[PATCHES] Cleanup dead records in messages.tdb

[Jeremy Allison]

On Thu, Apr 03, 2014 at 02:32:25PM -0700, Jeremy Allison wrote:
> On Thu, Apr 03, 2014 at 02:26:29PM -0700, Christof Schmitt wrote:
> > 
> > Actually, there is an issue with the locking here: tdb_delete fails,
> > because it tries to get a lock for the delete, but the chain is already
> > locked. Either we have to live with the race here, or add a call to tdb
> > that allows deletion without acquiring the lock.
> 
> Are you sure here ? That same bug would then exist
> in the function :
> 
> tatic NTSTATUS retrieve_all_messages(TDB_CONTEXT *msg_tdb,
>                                       struct server_id id,
>                                       TALLOC_CTX *mem_ctx,
>                                       struct messaging_array **presult)
> {
>         struct messaging_array *result;
>         TDB_DATA key = message_key_pid(mem_ctx, id);
>         NTSTATUS status;
> 
>         if (tdb_chainlock(msg_tdb, key) != 0) {
>                 TALLOC_FREE(key.dptr);
>                 return NT_STATUS_LOCK_NOT_GRANTED;
>         }
> 
>         status = messaging_tdb_fetch(msg_tdb, key, mem_ctx, &result);
> 
>         /*
>          * We delete the record here, tdb_set_max_dead keeps it around
>          */
>         tdb_delete(msg_tdb, key);
>         tdb_chainunlock(msg_tdb, key);
> 
>         if (NT_STATUS_IS_OK(status)) {
>                 *presult = result;
>         }
> 
>         TALLOC_FREE(key.dptr);
> 
>         return status;
> }
> 
> which is called regularly in the messaging
> code...

Just wrote a quick standalone test:

+static void jratest(TDB_CONTEXT *db)
+{
+       TDB_DATA key, data;
+
+       key.dptr = "keyname";
+       key.dsize = 8;
+       data.dptr = "data";
+       data.dsize = 5;
+
+       if (tdb_store(db, key, data, 0) != 0) {
+               abort();
+       }
+
+       if (tdb_chainlock(db, key) != 0) {
+               abort();
+       }
+
+       if (tdb_delete(db, key) != 0) {
+               abort();
+       }
+       if (tdb_chainunlock(db, key) != 0) {
+               abort();
+       }
+}

seems to work fine. So I don't think there's
a problem with deleting a record under a chainlock...

(I'm pretty sure those locks are recursive).

Jeremy.