The branch, v4-1-test has been updated
via fd1583b torture3: Trigger a nasty cleanup bug in smbd
via 3a5ae0c smbd: Fix flawed share_mode_stale_pid API
via 9cfc001 smbd: Rename parameter "i" to "idx"
via 252a2bc smbd: Don't store in-memory only flags in locking.tdb
via 1706214 smbd: Simplify find_oplock_types
from 4182c97 python-samba-tool fsmo: Do not give an error on a
successful role transfer
http://gitweb.samba.org/?p=samba.git;a=shortlog;h=v4-1-test
- Log -----------------------------------------------------------------
commit fd1583b3ce6d2275696e833f967e66f412367d5f
Author: Volker Lendecke <[email protected]>
Date: Sun Sep 1 18:54:59 2013 +0200
torture3: Trigger a nasty cleanup bug in smbd
Signed-off-by: Volker Lendecke <[email protected]>
Reviewed-by: Michael Adam <[email protected]>
Autobuild-User(master): Michael Adam <[email protected]>
Autobuild-Date(master): Tue Sep 3 19:13:14 CEST 2013 on sn-devel-104
(cherry picked from commit ade8477f98fcffcc6e3c5ea31618b49d0c1bba95)
The last 5 patches address bug #10138 - smbd doesn't always clean up share
modes
after hard crash.
Autobuild-User(v4-1-test): Karolin Seeger <[email protected]>
Autobuild-Date(v4-1-test): Thu Sep 26 11:33:29 CEST 2013 on sn-devel-104
commit 3a5ae0c11c4234950e6080a3b71f3d66db06edac
Author: Volker Lendecke <[email protected]>
Date: Fri Aug 30 12:49:43 2013 +0000
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 <[email protected]>
Reviewed-by: Michael Adam <[email protected]>
(cherry picked from commit 7d91ffc6fdc3b371564e14f09822a96264ea372a)
commit 9cfc00190882aea1915a721af2abbd07784ce6a7
Author: Volker Lendecke <[email protected]>
Date: Fri Aug 30 12:27:36 2013 +0000
smbd: Rename parameter "i" to "idx"
We'll need "i" in a later checkin ... :-)
Signed-off-by: Volker Lendecke <[email protected]>
Reviewed-by: Michael Adam <[email protected]>
(cherry picked from commit 5006db98aaf1efe119f1da8be091587a9bc2b952)
commit 252a2bc946de82697e49a015c60daf2fc5dee485
Author: Volker Lendecke <[email protected]>
Date: Sun Sep 1 11:07:19 2013 +0200
smbd: Don't store in-memory only flags in locking.tdb
Hey, pidl knows the [skip] attribute ... :-)
Signed-off-by: Volker Lendecke <[email protected]>
Reviewed-by: Michael Adam <[email protected]>
(cherry picked from commit 696bc569b17f024f840774e3d59761229836a310)
commit 17062140cc2977dc0877890928b25b798de3cacc
Author: Volker Lendecke <[email protected]>
Date: Thu Aug 22 08:49:07 2013 +0000
smbd: Simplify find_oplock_types
Signed-off-by: Volker Lendecke <[email protected]>
Reviewed-by: Michael Adam <[email protected]>
(cherry picked from commit 94b320527eee0c7ba1d3818816e7d59cb863bf3f)
-----------------------------------------------------------------------
Summary of changes:
source3/librpc/idl/open_files.idl | 10 ++++-
source3/locking/locking.c | 47 +++++++++++++++++--------
source3/locking/proto.h | 2 +-
source3/locking/share_mode_lock.c | 24 +++++++++++++
source3/selftest/tests.py | 1 +
source3/smbd/open.c | 19 +++++-----
source3/torture/proto.h | 1 +
source3/torture/test_cleanup.c | 70 +++++++++++++++++++++++++++++++++++++
source3/torture/torture.c | 1 +
9 files changed, 148 insertions(+), 27 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source3/librpc/idl/open_files.idl
b/source3/librpc/idl/open_files.idl
index fa87bc7..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 {
@@ -42,8 +48,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;
diff --git a/source3/locking/locking.c b/source3/locking/locking.c
index 7e65616..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,40 +634,53 @@ 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 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;
- 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/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);
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);
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/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 "
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},
--
Samba Shared Repository
