[SCM] Samba Shared Repository - branch master updated
Amitay Isaacs
amitay at samba.org
Fri Jun 12 07:29:04 MDT 2015
The branch, master has been updated
via b3a18d6 ctdb-locking: move all auto_mark logic into process_callbacks()
via a2690bc ctdb-locking: make process_callbacks() more robust
via 89849c4 ctdb-locking: Add a comment to explain auto_mark usage
via bc74703 ctdb-locking: Avoid resetting talloc destructor
via 2b352ff ctdb-locking: Avoid memory leak in the failure case
via 5ae6a8f ctdb-locking: Set destructor when lock_context is created
via 752ec31 ctdb-locking: Set the lock_ctx->request to NULL when request is freed
via ee02e40 ctdb-locking: Avoid memory corruption in ctdb_lock_context_destructor
from b73121f docs: Add missing SRVSVC entries in rpcclient manpage
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit b3a18d66c00dba73a3f56a6f95781b4d34db1fe2
Author: Stefan Metzmacher <metze at samba.org>
Date: Tue Jun 2 12:43:17 2015 +0200
ctdb-locking: move all auto_mark logic into process_callbacks()
The caller should not dereference lock_ctx after invoking
process_callbacks(), it might be destroyed already.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11293
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Amitay Isaacs <amitay at gmail.com>
Autobuild-User(master): Amitay Isaacs <amitay at samba.org>
Autobuild-Date(master): Fri Jun 12 15:28:57 CEST 2015 on sn-devel-104
commit a2690bc3f4e28a2ed50ccb47cb404fc8570fde6d
Author: Stefan Metzmacher <metze at samba.org>
Date: Tue Jun 2 12:39:17 2015 +0200
ctdb-locking: make process_callbacks() more robust
We should not dereference lock_ctx after invoking the callback
in the auto_mark == false case. The callback could have destroyed it.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11293
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Amitay Isaacs <amitay at gmail.com>
commit 89849c4d31c0bb0c47864e11abc89efe7d812d87
Author: Amitay Isaacs <amitay at gmail.com>
Date: Tue Jun 2 13:15:37 2015 +1000
ctdb-locking: Add a comment to explain auto_mark usage
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11293
Signed-off-by: Amitay Isaacs <amitay at gmail.com>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit bc747030d435447e62262541cf2e74be4c4229d8
Author: Amitay Isaacs <amitay at gmail.com>
Date: Tue Jun 2 11:25:44 2015 +1000
ctdb-locking: Avoid resetting talloc destructor
Let ctdb_lock_request_destructor() take care of the proper cleanup.
If the request if freed from the callback function, then the lock context
should not be freed. Setting request->lctx to NULL takes care of that
in the destructor.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11293
Signed-off-by: Amitay Isaacs <amitay at gmail.com>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit 2b352ff20597b9e34b3777d35deca1bf56209f8a
Author: Amitay Isaacs <amitay at gmail.com>
Date: Tue Jun 2 11:15:11 2015 +1000
ctdb-locking: Avoid memory leak in the failure case
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11293
Signed-off-by: Amitay Isaacs <amitay at gmail.com>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit 5ae6a8f2fff5b5f4d46f496fd83f555be4b3d448
Author: Amitay Isaacs <amitay at gmail.com>
Date: Tue Jun 2 00:22:07 2015 +1000
ctdb-locking: Set destructor when lock_context is created
There is already code in the destructor to correctly remove it from the
pending or the active queue. This also ensures that when lock context
is in pending queue and if the lock request gets freed, the lock context
is correctly removed from the pending queue.
Thanks to Stefan Metzmacher for noticing this and suggesting the fix.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11293
Signed-off-by: Amitay Isaacs <amitay at gmail.com>
Reviewed-by: Stefan Metzmacher <metze at samba.org>
commit 752ec31bcbbfe9f5b3b1c5dde4179d69f41cb53c
Author: Stefan Metzmacher <metze at samba.org>
Date: Tue Jun 2 00:15:11 2015 +1000
ctdb-locking: Set the lock_ctx->request to NULL when request is freed
The code was added to ctdb_lock_context_destructor() to ensure that
the if a lock_ctx gets freed first, the lock_request does not have a
dangling pointer. However, the reverse is also true. When a lock_request
is freed, then lock_ctx should not have a dangling pointer.
In commit 374cbc7b0ff68e04ee4e395935509c7df817b3c0, the code for second
condition was dropped causing a regression.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11293
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Amitay Isaacs <amitay at gmail.com>
commit ee02e40e869fd46f113d016122dd5384b7887228
Author: Stefan Metzmacher <metze at samba.org>
Date: Tue May 26 16:45:34 2015 +0200
ctdb-locking: Avoid memory corruption in ctdb_lock_context_destructor
If the lock request is freed from within the callback, then setting
lock_ctx->request to NULL in ctdb_lock_context_destructor will end up
corrupting memory. In this case, lock_ctx->request could be reallocated
and pointing to something else. This may cause unexpected abort trying
to dereference a NULL pointer.
So, set lock_ctx->request to NULL before processing callbacks.
This avoids the following valgrind problem.
==3636== Invalid write of size 8
==3636== at 0x151F3D: ctdb_lock_context_destructor (ctdb_lock.c:276)
==3636== by 0x58B3618: _talloc_free_internal (talloc.c:993)
==3636== by 0x58AD692: _talloc_free_children_internal (talloc.c:1472)
==3636== by 0x58AD692: _talloc_free_internal (talloc.c:1019)
==3636== by 0x58AD692: _talloc_free (talloc.c:1594)
==3636== by 0x15292E: ctdb_lock_handler (ctdb_lock.c:471)
==3636== by 0x56A535A: epoll_event_loop (tevent_epoll.c:728)
==3636== by 0x56A535A: epoll_event_loop_once (tevent_epoll.c:926)
==3636== by 0x56A3826: std_event_loop_once (tevent_standard.c:114)
==3636== by 0x569FFFC: _tevent_loop_once (tevent.c:533)
==3636== by 0x56A019A: tevent_common_loop_wait (tevent.c:637)
==3636== by 0x56A37C6: std_event_loop_wait (tevent_standard.c:140)
==3636== by 0x11E03A: ctdb_start_daemon (ctdb_daemon.c:1320)
==3636== by 0x118557: main (ctdbd.c:321)
==3636== Address 0x9c5b660 is 96 bytes inside a block of size 120 free'd
==3636== at 0x4C29D17: free (in
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==3636== by 0x58B32D3: _talloc_free_internal (talloc.c:1063)
==3636== by 0x58B3232: _talloc_free_children_internal (talloc.c:1472)
==3636== by 0x58B3232: _talloc_free_internal (talloc.c:1019)
==3636== by 0x58B3232: _talloc_free_children_internal (talloc.c:1472)
==3636== by 0x58B3232: _talloc_free_internal (talloc.c:1019)
==3636== by 0x58AD692: _talloc_free_children_internal (talloc.c:1472)
==3636== by 0x58AD692: _talloc_free_internal (talloc.c:1019)
==3636== by 0x58AD692: _talloc_free (talloc.c:1594)
==3636== by 0x11EC30: daemon_incoming_packet (ctdb_daemon.c:844)
==3636== by 0x136F4A: lock_fetch_callback (ctdb_ltdb_server.c:268)
==3636== by 0x152489: process_callbacks (ctdb_lock.c:353)
==3636== by 0x152489: ctdb_lock_handler (ctdb_lock.c:468)
==3636== by 0x56A535A: epoll_event_loop (tevent_epoll.c:728)
==3636== by 0x56A535A: epoll_event_loop_once (tevent_epoll.c:926)
==3636== by 0x56A3826: std_event_loop_once (tevent_standard.c:114)
==3636== by 0x569FFFC: _tevent_loop_once (tevent.c:533)
==3636== by 0x56A019A: tevent_common_loop_wait (tevent.c:637)
==3636== by 0x56A37C6: std_event_loop_wait (tevent_standard.c:140)
==3636== by 0x11E03A: ctdb_start_daemon (ctdb_daemon.c:1320)
==3636== by 0x118557: main (ctdbd.c:321)
BUG: https://bugzilla.samba.org/show_bug.cgi?id=11293
Signed-off-by: Stefan Metzmacher <metze at samba.org>
Reviewed-by: Amitay Isaacs <amitay at gmail.com>
-----------------------------------------------------------------------
Summary of changes:
ctdb/server/ctdb_lock.c | 49 ++++++++++++++++++++++++++++++-------------------
1 file changed, 30 insertions(+), 19 deletions(-)
Changeset truncated at 500 lines:
diff --git a/ctdb/server/ctdb_lock.c b/ctdb/server/ctdb_lock.c
index f592834..82295ea 100644
--- a/ctdb/server/ctdb_lock.c
+++ b/ctdb/server/ctdb_lock.c
@@ -41,6 +41,10 @@
* ctdb_lock_alldb() - get a lock on all DBs
*
* auto_mark - whether to mark/unmark DBs in before/after callback
+ * = false is used for freezing databases for
+ * recovery since the recovery cannot start till
+ * databases are locked on all the nodes.
+ * = true is used for record locks.
*/
enum lock_type {
@@ -312,7 +316,13 @@ static int ctdb_lock_context_destructor(struct lock_context *lock_ctx)
*/
static int ctdb_lock_request_destructor(struct lock_request *lock_request)
{
+ if (lock_request->lctx == NULL) {
+ return 0;
+ }
+
+ lock_request->lctx->request = NULL;
TALLOC_FREE(lock_request->lctx);
+
return 0;
}
@@ -324,8 +334,9 @@ static int ctdb_lock_request_destructor(struct lock_request *lock_request)
static void process_callbacks(struct lock_context *lock_ctx, bool locked)
{
struct lock_request *request;
+ bool auto_mark = lock_ctx->auto_mark;
- if (lock_ctx->auto_mark && locked) {
+ if (auto_mark && locked) {
switch (lock_ctx->type) {
case LOCK_RECORD:
tdb_chainlock_mark(lock_ctx->ctdb_db->ltdb->tdb, lock_ctx->key);
@@ -346,13 +357,23 @@ static void process_callbacks(struct lock_context *lock_ctx, bool locked)
}
request = lock_ctx->request;
- if (lock_ctx->auto_mark) {
- /* Reset the destructor, so request is not removed from the list */
- talloc_set_destructor(request, NULL);
+ if (auto_mark) {
+ /* Since request may be freed in the callback, unset the lock
+ * context, so request destructor will not free lock context.
+ */
+ request->lctx = NULL;
}
+
+ /* Since request may be freed in the callback, unset the request */
+ lock_ctx->request = NULL;
+
request->callback(request->private_data, locked);
- if (lock_ctx->auto_mark && locked) {
+ if (!auto_mark) {
+ return;
+ }
+
+ if (locked) {
switch (lock_ctx->type) {
case LOCK_RECORD:
tdb_chainlock_unmark(lock_ctx->ctdb_db->ltdb->tdb, lock_ctx->key);
@@ -371,6 +392,8 @@ static void process_callbacks(struct lock_context *lock_ctx, bool locked)
break;
}
}
+
+ talloc_free(lock_ctx);
}
@@ -416,7 +439,6 @@ static void ctdb_lock_handler(struct tevent_context *ev,
void *private_data)
{
struct lock_context *lock_ctx;
- TALLOC_CTX *tmp_ctx = NULL;
char c;
bool locked;
double t;
@@ -430,11 +452,6 @@ static void ctdb_lock_handler(struct tevent_context *ev,
t = timeval_elapsed(&lock_ctx->start_time);
id = lock_bucket_id(t);
- if (lock_ctx->auto_mark) {
- tmp_ctx = talloc_new(ev);
- talloc_steal(tmp_ctx, lock_ctx);
- }
-
/* Read the status from the child process */
if (sys_read(lock_ctx->fd[0], &c, 1) != 1) {
locked = false;
@@ -466,10 +483,6 @@ static void ctdb_lock_handler(struct tevent_context *ev,
}
process_callbacks(lock_ctx, locked);
-
- if (lock_ctx->auto_mark) {
- talloc_free(tmp_ctx);
- }
}
@@ -806,8 +819,6 @@ static void ctdb_lock_schedule(struct ctdb_context *ctdb)
/* Parent process */
close(lock_ctx->fd[1]);
- talloc_set_destructor(lock_ctx, ctdb_lock_context_destructor);
-
talloc_free(tmp_ctx);
/* Set up timeout handler */
@@ -819,7 +830,6 @@ static void ctdb_lock_schedule(struct ctdb_context *ctdb)
if (lock_ctx->ttimer == NULL) {
ctdb_kill(ctdb, lock_ctx->child, SIGKILL);
lock_ctx->child = -1;
- talloc_set_destructor(lock_ctx, NULL);
close(lock_ctx->fd[0]);
return;
}
@@ -835,7 +845,6 @@ static void ctdb_lock_schedule(struct ctdb_context *ctdb)
TALLOC_FREE(lock_ctx->ttimer);
ctdb_kill(ctdb, lock_ctx->child, SIGKILL);
lock_ctx->child = -1;
- talloc_set_destructor(lock_ctx, NULL);
close(lock_ctx->fd[0]);
return;
}
@@ -900,6 +909,7 @@ static struct lock_request *ctdb_lock_internal(TALLOC_CTX *mem_ctx,
if (lock_ctx->key.dptr == NULL) {
DEBUG(DEBUG_ERR, (__location__ "Memory allocation error\n"));
talloc_free(lock_ctx);
+ talloc_free(request);
return NULL;
}
lock_ctx->key_hash = ctdb_hash(&key);
@@ -933,6 +943,7 @@ static struct lock_request *ctdb_lock_internal(TALLOC_CTX *mem_ctx,
request->private_data = private_data;
talloc_set_destructor(request, ctdb_lock_request_destructor);
+ talloc_set_destructor(lock_ctx, ctdb_lock_context_destructor);
ctdb_lock_schedule(ctdb);
--
Samba Shared Repository
More information about the samba-cvs
mailing list