From 0bfb0787194184ac154e4d5c9d16cc0b9e84e007 Mon Sep 17 00:00:00 2001 From: Michael Adam Date: Thu, 20 Mar 2014 12:07:19 +0100 Subject: [PATCH] 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 Reviewed-by: Jeremy Allison --- source3/winbindd/idmap_autorid_tdb.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-) diff --git a/source3/winbindd/idmap_autorid_tdb.c b/source3/winbindd/idmap_autorid_tdb.c index 15d01923234..e01c31bc7c8 100644 --- 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))); -- 2.11.4.GIT