Possible fixes for bug 11293 - invalid write in ctdb_lock_context_destructor

[Stefan (metze) Metzmacher]

Hi Amitay,

can you have a look at the following patches, we may squash them...

The code is a bit hard to understand and the removing of the destructor
(see the 2nd commit) is not clear to me. It's also unclear what the lifetime
of these structure is.

metze
-------------- next part --------------
From 847f99beae228f7e7fcb37a34a1584a1cd6fbf3d Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 26 May 2015 16:22:26 +0200
Subject: [PATCH 1/2] ctdb_lock: fix the interaction between
 ctdb_lock_request_destructor and ctdb_lock_context_destructor

The hopefully 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)

Signed-off-by: Stefan Metzmacher <metze at samba.org>
---
 ctdb/server/ctdb_lock.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ctdb/server/ctdb_lock.c b/ctdb/server/ctdb_lock.c
index df22d0d..9db9efa 100644
--- a/ctdb/server/ctdb_lock.c
+++ b/ctdb/server/ctdb_lock.c
@@ -312,7 +312,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;
 }
 
-- 
1.9.1


From c98eedd2ade5eedf2a7063c0f9019e04ec8faeab Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Tue, 26 May 2015 16:45:34 +0200
Subject: [PATCH 2/2] ctdb_lock: TODO process_callbacks clear lock_ctx->request

---
 ctdb/server/ctdb_lock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ctdb/server/ctdb_lock.c b/ctdb/server/ctdb_lock.c
index 9db9efa..85d4e9d 100644
--- a/ctdb/server/ctdb_lock.c
+++ b/ctdb/server/ctdb_lock.c
@@ -353,6 +353,7 @@ static void process_callbacks(struct lock_context *lock_ctx, bool locked)
 
 	request = lock_ctx->request;
 	if (lock_ctx->auto_mark) {
+		lock_ctx->request = NULL;
 		/* Reset the destructor, so request is not removed from the list */
 		talloc_set_destructor(request, NULL);
 	}
-- 
1.9.1
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150527/0eb4849e/attachment.pgp>