autorid: fix a potential for data corruption.

The initialization of the HWM values in autorid.tdb was racy:

It did:

1. fetch the HWM value
2. if it did not exist, store 0 in a transaction.

This can be racy if two processes at the same time try to
run the initialization code, especially in a cluster, when
winbindd and smbd are started simultaneously on all nodes.
The race is that the HWM is not re-fetched inside the transaction.

Assume both processes see that the HWM does not exist.
Both try to start a transaction. Process 1 gets the lock
and process 2 blocks. After Process 1 has stored the
HWM, it proceeds and manages to start subsequent transactions
which also bump the HWM value (e.g. a range allocation,
which is also triggered from allocation code). When
process 2 finally manages to start the transaction, the
HWM value is aready > 0. But process 2 does not look again
and simply overwrites the HWM with 0.

So the next allocation will overwrite an existing mapping,
at least partially.

This patch changes the mechanism to:

1. fetch the hwm value
2. if it does not exist start a transaction
3.   fetch the hwm value
4.   if it does not exist, store 0
5. commit the transaction.

Note: this is not theoretical. Corruptions have been
seen in cluster environments.

Signed-off-by: Michael Adam <obnox@samba.org>
Reviewed-by: Jeremy Allison <jra@samba.org>

diff --git a/source3/winbindd/idmap_autorid_tdb.c b/source3/winbindd/idmap_autorid_tdb.c
index 15d0192323411bd48e1b7f3d3e1ce3f8b7ecb4ed..e01c31bc7c83f91a252916c1587ef5a8965f885c 100644 (file)
--- a/source3/winbindd/idmap_autorid_tdb.c
+++ b/source3/winbindd/idmap_autorid_tdb.c
@@ -389,6 +389,37 @@ NTSTATUS idmap_autorid_get_domainrange(struct db_context *db,
 }
 
 /* initialize the given HWM to 0 if it does not exist yet */
+static NTSTATUS idmap_autorid_init_hwm_action(struct db_context *db,
+                                             void *private_data)
+{
+       NTSTATUS status;
+       uint32_t hwmval;
+       const char *hwm;
+
+       hwm = (char *)private_data;
+
+       status = dbwrap_fetch_uint32_bystring(db, hwm, &hwmval);
+       if (NT_STATUS_IS_OK(status)) {
+               DEBUG(1, ("HWM (%s) already initialized in autorid database "
+                         "(value %"PRIu32").\n", hwm, hwmval));
+               return NT_STATUS_OK;
+       }
+       if (!NT_STATUS_EQUAL(status, NT_STATUS_NOT_FOUND)) {
+               DEBUG(0, ("Error fetching HWM (%s) from autorid "
+                         "database: %s\n", hwm, nt_errstr(status)));
+               return status;
+       }
+
+       status = dbwrap_trans_store_uint32_bystring(db, hwm, 0);
+       if (!NT_STATUS_IS_OK(status)) {
+               DEBUG(0, ("Error storing HWM (%s) in autorid database: %s\n",
+                         hwm, nt_errstr(status)));
+               return status;
+       }
+
+       return NT_STATUS_OK;
+}
+
 NTSTATUS idmap_autorid_init_hwm(struct db_context *db, const char *hwm)
 {
        NTSTATUS status;
@@ -406,7 +437,8 @@ NTSTATUS idmap_autorid_init_hwm(struct db_context *db, const char *hwm)
                return status;
        }
 
-       status = dbwrap_trans_store_uint32_bystring(db, hwm, 0);
+       status = dbwrap_trans_do(db, idmap_autorid_init_hwm_action,
+                                (void *)hwm);
        if (!NT_STATUS_IS_OK(status)) {
                DEBUG(0, ("Error initializing HWM (%s) in autorid database: "
                          "%s\n", hwm, nt_errstr(status)));