Patchset: Fix a cleanup bug
Volker Lendecke
Volker.Lendecke at SerNet.DE
Sun Sep 1 13:24:48 MDT 2013
Hi!
While staring at code I found a nasty defect that might kick
in when an smbd dies hard and leaves stale share mode
entries behind. See the torture test in the last patch of
this patchset.
Please review and comment or push.
Thanks,
Volker
--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: +49-551-370000-0, fax: +49-551-370000-9
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:kontakt at sernet.de
*****************************************************************
visit us on it-sa:IT security exhibitions in Nürnberg, Germany
October 8th - 10th 2013, hall 12, booth 333
free tickets available via code 270691 on: www.it-sa.de/gutschein
******************************************************************
-------------- next part --------------
>From 9bdbb46d89b6b76529e92a37cabc85d3f2b42bbf Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Thu, 22 Aug 2013 08:49:07 +0000
Subject: [PATCH 2/6] smbd: Simplify find_oplock_types
Signed-off-by: Volker Lendecke <vl at samba.org>
---
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 53f8b8e..c28d2a3 100644
--- a/source3/smbd/open.c
+++ b/source3/smbd/open.c
@@ -1237,19 +1237,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"));
@@ -1258,10 +1259,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;
@@ -1270,10 +1271,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 "
@@ -1285,7 +1286,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.7.9.5
>From 12d65ae7c30e1ad407e9b0caa291adbdbe13fa82 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 1 Sep 2013 11:07:19 +0200
Subject: [PATCH 3/6] smbd: Don't store in-memory only flags in locking.tdb
Hey, pidl knows the [skip] attribute ... :-)
Signed-off-by: Volker Lendecke <vl at samba.org>
---
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.7.9.5
>From 9850bf107c55eff36e2c4188912bc212635624fe Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Fri, 30 Aug 2013 12:27:36 +0000
Subject: [PATCH 4/6] smbd: Rename parameter "i" to "idx"
We'll need "i" in a later checkin ... :-)
Signed-off-by: Volker Lendecke <vl at samba.org>
---
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.7.9.5
>From 84608afa8473223169eb2a981d70db4015b83e2c Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Fri, 30 Aug 2013 12:49:43 +0000
Subject: [PATCH 5/6] 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 at samba.org>
---
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.7.9.5
>From 45446892cc9cac1ad1390f88f821e5aafae828b3 Mon Sep 17 00:00:00 2001
From: Volker Lendecke <vl at samba.org>
Date: Sun, 1 Sep 2013 18:54:59 +0200
Subject: [PATCH 6/6] torture3: Trigger a nasty cleanup bug in smbd
Signed-off-by: Volker Lendecke <vl at samba.org>
---
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 afab687..63f119f 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 f6453fd..d430aef 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 359baa9..2d1a107 100644
--- a/source3/torture/torture.c
+++ b/source3/torture/torture.c
@@ -9554,6 +9554,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.7.9.5
More information about the samba-technical
mailing list