From 4afbb4a94f6b259544728723291bece59ed59231 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl@samba.org>
Date: Thu, 22 Aug 2013 08:49:07 +0000
Subject: [PATCH 1/5] smbd: Simplify find_oplock_types

Signed-off-by: Volker Lendecke <vl@samba.org>
Reviewed-by: Michael Adam <obnox@samba.org>
(cherry picked from commit 94b320527eee0c7ba1d3818816e7d59cb863bf3f)
---
 source3/smbd/open.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/source3/smbd/open.c b/source3/smbd/open.c
index dcf6bb5..10b04e7 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1251,19 +1251,20 @@ static void find_oplock_types(files_struct *fsp,
 	}
 
 	for (i=0; i<lck->data->num_share_modes; i++) {
-		if (!is_valid_share_mode_entry(&lck->data->share_modes[i])) {
+		struct share_mode_entry *e = &lck->data->share_modes[i];
+
+		if (!is_valid_share_mode_entry(e)) {
 			continue;
 		}
 
-		if (lck->data->share_modes[i].op_type == NO_OPLOCK &&
-				is_stat_open(lck->data->share_modes[i].access_mask)) {
+		if (e->op_type == NO_OPLOCK && is_stat_open(e->access_mask)) {
 			/* We ignore stat opens in the table - they
 			   always have NO_OPLOCK and never get or
 			   cause breaks. JRA. */
 			continue;
 		}
 
-		if (BATCH_OPLOCK_TYPE(lck->data->share_modes[i].op_type)) {
+		if (BATCH_OPLOCK_TYPE(e->op_type)) {
 			/* batch - can only be one. */
 			if (share_mode_stale_pid(lck->data, i)) {
 				DEBUG(10, ("Found stale batch oplock\n"));
@@ -1272,10 +1273,10 @@ static void find_oplock_types(files_struct *fsp,
 			if (*pp_ex_or_batch || *pp_batch || *got_level2 || *got_no_oplock) {
 				smb_panic("Bad batch oplock entry.");
 			}
-			*pp_batch = &lck->data->share_modes[i];
+			*pp_batch = e;
 		}
 
-		if (EXCLUSIVE_OPLOCK_TYPE(lck->data->share_modes[i].op_type)) {
+		if (EXCLUSIVE_OPLOCK_TYPE(e->op_type)) {
 			if (share_mode_stale_pid(lck->data, i)) {
 				DEBUG(10, ("Found stale duplicate oplock\n"));
 				continue;
@@ -1284,10 +1285,10 @@ static void find_oplock_types(files_struct *fsp,
 			if (*pp_ex_or_batch || *got_level2 || *got_no_oplock) {
 				smb_panic("Bad exclusive or batch oplock entry.");
 			}
-			*pp_ex_or_batch = &lck->data->share_modes[i];
+			*pp_ex_or_batch = e;
 		}
 
-		if (LEVEL_II_OPLOCK_TYPE(lck->data->share_modes[i].op_type)) {
+		if (LEVEL_II_OPLOCK_TYPE(e->op_type)) {
 			if (*pp_batch || *pp_ex_or_batch) {
 				if (share_mode_stale_pid(lck->data, i)) {
 					DEBUG(10, ("Found stale LevelII "
@@ -1299,7 +1300,7 @@ static void find_oplock_types(files_struct *fsp,
 			*got_level2 = true;
 		}
 
-		if (lck->data->share_modes[i].op_type == NO_OPLOCK) {
+		if (e->op_type == NO_OPLOCK) {
 			if (*pp_batch || *pp_ex_or_batch) {
 				if (share_mode_stale_pid(lck->data, i)) {
 					DEBUG(10, ("Found stale NO_OPLOCK "
-- 
1.8.4


From a3d150c6eb9fc6419579f3934e9c098447d3d7fd Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl@samba.org>
Date: Sun, 1 Sep 2013 11:07:19 +0200
Subject: [PATCH 2/5] smbd: Don't store in-memory only flags in locking.tdb

Hey, pidl knows the [skip] attribute ... :-)

Signed-off-by: Volker Lendecke <vl@samba.org>
Reviewed-by: Michael Adam <obnox@samba.org>
(cherry picked from commit 696bc569b17f024f840774e3d59761229836a310)
---
 source3/librpc/idl/open_files.idl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/source3/librpc/idl/open_files.idl b/source3/librpc/idl/open_files.idl
index fa87bc7..a2e386f 100644
--- a/source3/librpc/idl/open_files.idl
+++ b/source3/librpc/idl/open_files.idl
@@ -42,8 +42,8 @@ interface open_files
 		[size_is(num_delete_tokens)] delete_token delete_tokens[];
 		timespec old_write_time;
 		timespec changed_write_time;
-		uint8 fresh;
-		uint8 modified;
+		[skip] boolean8 fresh;
+		[skip] boolean8 modified;
 		[ignore] db_record *record;
 	} share_mode_data;
 
-- 
1.8.4


From 64d66d3f5363942e6ac0fdabd062dcbc861eda4e Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl@samba.org>
Date: Fri, 30 Aug 2013 12:27:36 +0000
Subject: [PATCH 3/5] smbd: Rename parameter "i" to "idx"

We'll need "i" in a later checkin ... :-)

Signed-off-by: Volker Lendecke <vl@samba.org>
Reviewed-by: Michael Adam <obnox@samba.org>
(cherry picked from commit 5006db98aaf1efe119f1da8be091587a9bc2b952)
---
 source3/locking/locking.c | 12 ++++++------
 source3/locking/proto.h   |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/source3/locking/locking.c b/source3/locking/locking.c
index 7e65616..602c30d 100644
--- a/source3/locking/locking.c
+++ b/source3/locking/locking.c
@@ -634,24 +634,24 @@ bool is_valid_share_mode_entry(const struct share_mode_entry *e)
  * from d->share_modes. Modifies d->num_share_modes, watch out in
  * routines iterating over that array.
  */
-bool share_mode_stale_pid(struct share_mode_data *d, unsigned i)
+bool share_mode_stale_pid(struct share_mode_data *d, unsigned idx)
 {
 	struct share_mode_entry *e;
 
-	if (i > d->num_share_modes) {
+	if (idx > d->num_share_modes) {
 		DEBUG(1, ("Asking for index %u, only %u around\n",
-			  i, (unsigned)d->num_share_modes));
+			  idx, (unsigned)d->num_share_modes));
 		return false;
 	}
-	e = &d->share_modes[i];
+	e = &d->share_modes[idx];
 	if (serverid_exists(&e->pid)) {
 		DEBUG(10, ("PID %s (index %u out of %u) still exists\n",
-			   procid_str_static(&e->pid), i,
+			   procid_str_static(&e->pid), idx,
 			   (unsigned)d->num_share_modes));
 		return false;
 	}
 	DEBUG(10, ("PID %s (index %u out of %u) does not exist anymore\n",
-		   procid_str_static(&e->pid), i,
+		   procid_str_static(&e->pid), idx,
 		   (unsigned)d->num_share_modes));
 	*e = d->share_modes[d->num_share_modes-1];
 	d->num_share_modes -= 1;
diff --git a/source3/locking/proto.h b/source3/locking/proto.h
index adb30b7..93fbea5 100644
--- a/source3/locking/proto.h
+++ b/source3/locking/proto.h
@@ -170,7 +170,7 @@ void get_file_infos(struct file_id id,
 		    bool *delete_on_close,
 		    struct timespec *write_time);
 bool is_valid_share_mode_entry(const struct share_mode_entry *e);
-bool share_mode_stale_pid(struct share_mode_data *d, unsigned i);
+bool share_mode_stale_pid(struct share_mode_data *d, unsigned idx);
 void set_share_mode(struct share_mode_lock *lck, files_struct *fsp,
 		    uid_t uid, uint64_t mid, uint16 op_type);
 bool del_share_mode(struct share_mode_lock *lck, files_struct *fsp);
-- 
1.8.4


From da5b75e8efc878191f3867a150937fcf91052dfe Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl@samba.org>
Date: Fri, 30 Aug 2013 12:49:43 +0000
Subject: [PATCH 4/5] smbd: Fix flawed share_mode_stale_pid API

The comment for this routine said:

> Modifies d->num_share_modes, watch out in routines iterating over
> that array.

Well, it turns out that *every* caller of this API got it wrong. So I
think it's better to change the routine.

This leaves the array untouched while iterating but filters out the
deleted ones while saving them back to disk.

Signed-off-by: Volker Lendecke <vl@samba.org>
Reviewed-by: Michael Adam <obnox@samba.org>
(cherry picked from commit 7d91ffc6fdc3b371564e14f09822a96264ea372a)
---
 source3/librpc/idl/open_files.idl |  6 ++++++
 source3/locking/locking.c         | 35 ++++++++++++++++++++++++++---------
 source3/locking/share_mode_lock.c | 24 ++++++++++++++++++++++++
 3 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/source3/librpc/idl/open_files.idl b/source3/librpc/idl/open_files.idl
index a2e386f..686bc02 100644
--- a/source3/librpc/idl/open_files.idl
+++ b/source3/librpc/idl/open_files.idl
@@ -23,6 +23,12 @@ interface open_files
 		uint32		uid;
 		uint16		flags;
 		uint32		name_hash;
+
+		/*
+		 * In-memory flag indicating a non-existing pid. We don't want
+		 * to store this share_mode_entry on disk.
+		 */
+		[skip] boolean8	stale;
 	} share_mode_entry;
 
 	typedef [public] struct {
diff --git a/source3/locking/locking.c b/source3/locking/locking.c
index 602c30d..5090082 100644
--- a/source3/locking/locking.c
+++ b/source3/locking/locking.c
@@ -617,6 +617,10 @@ bool is_valid_share_mode_entry(const struct share_mode_entry *e)
 {
 	int num_props = 0;
 
+	if (e->stale) {
+		return false;
+	}
+
 	num_props += ((e->op_type == NO_OPLOCK) ? 1 : 0);
 	num_props += (EXCLUSIVE_OPLOCK_TYPE(e->op_type) ? 1 : 0);
 	num_props += (LEVEL_II_OPLOCK_TYPE(e->op_type) ? 1 : 0);
@@ -630,9 +634,7 @@ bool is_valid_share_mode_entry(const struct share_mode_entry *e)
 /*
  * In case d->share_modes[i] conflicts with something or otherwise is
  * being used, we need to make sure the corresponding process still
- * exists. This routine checks it and potentially removes the entry
- * from d->share_modes. Modifies d->num_share_modes, watch out in
- * routines iterating over that array.
+ * exists.
  */
 bool share_mode_stale_pid(struct share_mode_data *d, unsigned idx)
 {
@@ -653,17 +655,32 @@ bool share_mode_stale_pid(struct share_mode_data *d, unsigned idx)
 	DEBUG(10, ("PID %s (index %u out of %u) does not exist anymore\n",
 		   procid_str_static(&e->pid), idx,
 		   (unsigned)d->num_share_modes));
-	*e = d->share_modes[d->num_share_modes-1];
-	d->num_share_modes -= 1;
 
-	if (d->num_share_modes == 0 &&
-	    d->num_delete_tokens) {
+	e->stale = true;
+
+	if (d->num_delete_tokens != 0) {
+		uint32_t i, num_stale;
+
 		/*
 		 * We cannot have any delete tokens
 		 * if there are no valid share modes.
 		 */
-		TALLOC_FREE(d->delete_tokens);
-		d->num_delete_tokens = 0;
+
+		num_stale = 0;
+
+		for (i=0; i<d->num_share_modes; i++) {
+			if (d->share_modes[i].stale) {
+				num_stale += 1;
+			}
+		}
+
+		if (num_stale == d->num_share_modes) {
+			/*
+			 * No non-stale share mode found
+			 */
+			TALLOC_FREE(d->delete_tokens);
+			d->num_delete_tokens = 0;
+		}
 	}
 
 	d->modified = true;
diff --git a/source3/locking/share_mode_lock.c b/source3/locking/share_mode_lock.c
index 0693cf5..342f910 100644
--- a/source3/locking/share_mode_lock.c
+++ b/source3/locking/share_mode_lock.c
@@ -121,6 +121,7 @@ static struct share_mode_data *parse_share_modes(TALLOC_CTX *mem_ctx,
 {
 	struct share_mode_data *d;
 	enum ndr_err_code ndr_err;
+	uint32_t i;
 	DATA_BLOB blob;
 
 	d = talloc(mem_ctx, struct share_mode_data);
@@ -140,6 +141,14 @@ static struct share_mode_data *parse_share_modes(TALLOC_CTX *mem_ctx,
 		goto fail;
 	}
 
+	/*
+	 * Initialize the values that are [skip] in the idl. The NDR code does
+	 * not initialize them.
+	 */
+
+	for (i=0; i<d->num_share_modes; i++) {
+		d->share_modes[i].stale = false;
+	}
 	d->modified = false;
 	d->fresh = false;
 
@@ -162,12 +171,27 @@ static TDB_DATA unparse_share_modes(struct share_mode_data *d)
 {
 	DATA_BLOB blob;
 	enum ndr_err_code ndr_err;
+	uint32_t i;
 
 	if (DEBUGLEVEL >= 10) {
 		DEBUG(10, ("unparse_share_modes:\n"));
 		NDR_PRINT_DEBUG(share_mode_data, d);
 	}
 
+	i = 0;
+	while (i < d->num_share_modes) {
+		if (d->share_modes[i].stale) {
+			/*
+			 * Remove the stale entries before storing
+			 */
+			struct share_mode_entry *m = d->share_modes;
+			m[i] = m[d->num_share_modes-1];
+			d->num_share_modes -= 1;
+		} else {
+			i += 1;
+		}
+	}
+
 	if (d->num_share_modes == 0) {
 		DEBUG(10, ("No used share mode found\n"));
 		return make_tdb_data(NULL, 0);
-- 
1.8.4


From 5cb1dfe73dfa06dd1fdcf8e58370085098f9c838 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl@samba.org>
Date: Sun, 1 Sep 2013 18:54:59 +0200
Subject: [PATCH 5/5] torture3: Trigger a nasty cleanup bug in smbd

Signed-off-by: Volker Lendecke <vl@samba.org>
Reviewed-by: Michael Adam <obnox@samba.org>

Autobuild-User(master): Michael Adam <obnox@samba.org>
Autobuild-Date(master): Tue Sep  3 19:13:14 CEST 2013 on sn-devel-104

(cherry picked from commit ade8477f98fcffcc6e3c5ea31618b49d0c1bba95)
---
 source3/selftest/tests.py      |  1 +
 source3/torture/proto.h        |  1 +
 source3/torture/test_cleanup.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 source3/torture/torture.c      |  1 +
 4 files changed, 73 insertions(+)

diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 3fc6684..e5ae63e 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -63,6 +63,7 @@ tests = ["FDPASS", "LOCK1", "LOCK2", "LOCK3", "LOCK4", "LOCK5", "LOCK6", "LOCK7"
         "SMB2-SESSION-REAUTH", "SMB2-SESSION-RECONNECT",
         "CLEANUP1",
         "CLEANUP2",
+        "CLEANUP4",
         "BAD-NBT-SESSION"]
 
 for t in tests:
diff --git a/source3/torture/proto.h b/source3/torture/proto.h
index 4f4c9e2..c9fc2c5 100644
--- a/source3/torture/proto.h
+++ b/source3/torture/proto.h
@@ -104,6 +104,7 @@ bool run_local_sprintf_append(int dummy);
 bool run_cleanup1(int dummy);
 bool run_cleanup2(int dummy);
 bool run_cleanup3(int dummy);
+bool run_cleanup4(int dummy);
 bool run_ctdb_conn(int dummy);
 bool run_msg_test(int dummy);
 bool run_notify_bench2(int dummy);
diff --git a/source3/torture/test_cleanup.c b/source3/torture/test_cleanup.c
index d9dce40..319a55f 100644
--- a/source3/torture/test_cleanup.c
+++ b/source3/torture/test_cleanup.c
@@ -329,3 +329,73 @@ bool run_cleanup3(int dummy)
 
 	return true;
 }
+
+bool run_cleanup4(int dummy)
+{
+	struct cli_state *cli1, *cli2;
+	const char *fname = "\\cleanup4";
+	uint16_t fnum1, fnum2;
+	NTSTATUS status;
+
+	printf("CLEANUP4: Checking that a conflicting share mode is cleaned "
+	       "up\n");
+
+	if (!torture_open_connection(&cli1, 0)) {
+		return false;
+	}
+	if (!torture_open_connection(&cli2, 0)) {
+		return false;
+	}
+
+	status = cli_ntcreate(
+		cli1, fname, 0,
+		FILE_GENERIC_READ|DELETE_ACCESS,
+		FILE_ATTRIBUTE_NORMAL,
+		FILE_SHARE_READ|FILE_SHARE_DELETE,
+		FILE_OVERWRITE_IF, 0, 0, &fnum1);
+	if (!NT_STATUS_IS_OK(status)) {
+		printf("creating file failed: %s\n",
+		       nt_errstr(status));
+		return false;
+	}
+
+	status = cli_ntcreate(
+		cli2, fname, 0,
+		FILE_GENERIC_READ|DELETE_ACCESS,
+		FILE_ATTRIBUTE_NORMAL,
+		FILE_SHARE_READ|FILE_SHARE_DELETE,
+		FILE_OPEN, 0, 0, &fnum2);
+	if (!NT_STATUS_IS_OK(status)) {
+		printf("opening file 1st time failed: %s\n",
+		       nt_errstr(status));
+		return false;
+	}
+
+	status = smbXcli_conn_samba_suicide(cli1->conn, 1);
+	if (!NT_STATUS_IS_OK(status)) {
+		printf("smbXcli_conn_samba_suicide failed: %s\n",
+		       nt_errstr(status));
+		return false;
+	}
+
+	/*
+	 * The next open will conflict with both opens above. The first open
+	 * above will be correctly cleaned up. A bug in smbd iterating over
+	 * the share mode array made it skip the share conflict check for the
+	 * second open. Trigger this bug.
+	 */
+
+	status = cli_ntcreate(
+		cli2, fname, 0,
+		FILE_GENERIC_WRITE|DELETE_ACCESS,
+		FILE_ATTRIBUTE_NORMAL,
+		FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE,
+		FILE_OPEN, 0, 0, &fnum2);
+	if (!NT_STATUS_EQUAL(status, NT_STATUS_SHARING_VIOLATION)) {
+		printf("opening file 2nd time returned: %s\n",
+		       nt_errstr(status));
+		return false;
+	}
+
+	return true;
+}
diff --git a/source3/torture/torture.c b/source3/torture/torture.c
index c6c5322..ee51a4d 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -9509,6 +9509,7 @@ static struct {
 	{ "CLEANUP1", run_cleanup1 },
 	{ "CLEANUP2", run_cleanup2 },
 	{ "CLEANUP3", run_cleanup3 },
+	{ "CLEANUP4", run_cleanup4 },
 	{ "LOCAL-SUBSTITUTE", run_local_substitute, 0},
 	{ "LOCAL-GENCACHE", run_local_gencache, 0},
 	{ "LOCAL-TALLOC-DICT", run_local_talloc_dict, 0},
-- 
1.8.4