serverid patchset ?

Michael Adam obnox at samba.org
Thu May 24 06:37:43 MDT 2012


Hi Jeremy,

In general, the patchset looks very good. Just a few comments,
mostly on patch cosmetics:

1. Patch #13 should (or could) be squashed with patch #1.
   (it only improves the functin added in patch #1)
   (It could then take a Pair-Programmed-With tag ;)

2. Adding the test (current patch #7)
   and running the test (curent patch #12) should be
   the last two patches

3. Adding the new checks should all come before the removal of
   of the orignal check from parse_share_modes() in patch #5.
   I.e. patch #5 should be moved to be the last patch before
   adding the test (i.e the third from the end).

4. The comment and if statement in (current) patch #13 should be
   reformatted to adhere to our coding style guidelines.

5. The comment in current patch #14 should be reformatted to adhere
   to coding guidelines.

6. I am not 100% certain about the complete correctnes of the
   changed file_existed handling in patch #14. I assume you
   checked it thoroughly...

Cheers - Michael


Jeremy Allison wrote:
> On Tue, May 22, 2012 at 04:18:54PM +0200, Stefan (metze) Metzmacher wrote:
> > 
> > I think we found the problem with DELETE_PENDING.
> > 
> > The code in open_mode_check() looks like this:
> > 
> >         int i;
> > 
> >         if(lck->data->num_share_modes == 0) {
> >                 return NT_STATUS_OK;
> >         }
> > 
> >         *file_existed = True;
> > 
> >         /* A delete on close prohibits everything */
> > 
> >         if (is_delete_on_close_set(lck, name_hash)) {
> >                 return NT_STATUS_DELETE_PENDING;
> >         }
> > 
> > Before parse_share_modes() cleaned up stale share mode entries,
> > as result lck->data->num_share_modes is 0 and we can return NT_STATUS_OK.
> 
> Ok, here is Volker's latest patchset with a couple of
> patches layered on top to fix this issue.
> 
> Currently testing - but more eyes welcome. I think
> we're getting there..
> 
> Cheers,
> 
> 	Jeremy.

> >From 87afcffdf26e87ac4f601bc247055dde2f0e3ca7 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 May 2012 12:57:07 +0200
> Subject: [PATCH 01/14] s3: Add "share_mode_stale_pid"
> 
> This is a helper routine that prunes a dead share mode entry on demand. This
> prepares for removing the serverids_exist call in parse_share_modes.
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/locking/locking.c |   32 ++++++++++++++++++++++++++++++++
>  source3/locking/proto.h   |    1 +
>  2 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/source3/locking/locking.c b/source3/locking/locking.c
> index b9afd23..df21104 100644
> --- a/source3/locking/locking.c
> +++ b/source3/locking/locking.c
> @@ -625,6 +625,38 @@ bool is_deferred_open_entry(const struct share_mode_entry *e)
>  	return (e->op_type == DEFERRED_OPEN_ENTRY);
>  }
>  
> +/*
> + * 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.
> + */
> +bool share_mode_stale_pid(struct share_mode_data *d, unsigned i)
> +{
> +	struct share_mode_entry *e;
> +
> +	if (i > d->num_share_modes) {
> +		DEBUG(1, ("Asking for index %u, only %u around\n",
> +			  i, (unsigned)d->num_share_modes));
> +		return false;
> +	}
> +	e = &d->share_modes[i];
> +	if (serverid_exists(&e->pid)) {
> +		DEBUG(10, ("PID %s (index %u out of %u) still exists\n",
> +			   procid_str_static(&e->pid), i,
> +			   (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,
> +		   (unsigned)d->num_share_modes));
> +	*e = d->share_modes[d->num_share_modes-1];
> +	d->num_share_modes -= 1;
> +	d->modified = true;
> +	return true;
> +}
> +
>  /*******************************************************************
>   Fill a share mode entry.
>  ********************************************************************/
> diff --git a/source3/locking/proto.h b/source3/locking/proto.h
> index 54badd9..f6a6f2e 100644
> --- a/source3/locking/proto.h
> +++ b/source3/locking/proto.h
> @@ -168,6 +168,7 @@ void get_file_infos(struct file_id id,
>  		    struct timespec *write_time);
>  bool is_valid_share_mode_entry(const struct share_mode_entry *e);
>  bool is_deferred_open_entry(const struct share_mode_entry *e);
> +bool share_mode_stale_pid(struct share_mode_data *d, unsigned i);
>  void set_share_mode(struct share_mode_lock *lck, files_struct *fsp,
>  		    uid_t uid, uint64_t mid, uint16 op_type);
>  void add_deferred_open(struct share_mode_lock *lck, uint64_t mid,
> -- 
> 1.7.7.3
> 
> 
> >From 74326527d306b8ae390ae0073d32cf02ebbfe609 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 May 2012 12:22:50 +0200
> Subject: [PATCH 02/14] s3: Check for serverid_exists in notify_deferred_opens
> 
> We will remove the check in parse_share_modes soon
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/smbd/close.c |   10 ++++++++--
>  1 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/source3/smbd/close.c b/source3/smbd/close.c
> index ede7925..7ecda5c 100644
> --- a/source3/smbd/close.c
> +++ b/source3/smbd/close.c
> @@ -179,9 +179,15 @@ static void notify_deferred_opens(struct smbd_server_connection *sconn,
>  
>  	num_deferred = 0;
>  	for (i=0; i<lck->data->num_share_modes; i++) {
> -		if (is_deferred_open_entry(&lck->data->share_modes[i])) {
> -			num_deferred += 1;
> +		struct share_mode_entry *e = &lck->data->share_modes[i];
> +
> +		if (!is_deferred_open_entry(e)) {
> +			continue;
> +		}
> +		if (share_mode_stale_pid(lck->data, i)) {
> +			continue;
>  		}
> +		num_deferred += 1;
>  	}
>  	if (num_deferred == 0) {
>  		return;
> -- 
> 1.7.7.3
> 
> 
> >From ed94d270e3ea454b32b241d3e1c17e3ecdf068cf Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 May 2012 15:23:10 +0200
> Subject: [PATCH 03/14] s3: Check for serverid_exists in open_mode_check
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/smbd/open.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/source3/smbd/open.c b/source3/smbd/open.c
> index 543a661..8a5273e 100644
> --- a/source3/smbd/open.c
> +++ b/source3/smbd/open.c
> @@ -1024,6 +1024,11 @@ static NTSTATUS open_mode_check(connection_struct *conn,
>  		 * too */
>  		if (share_conflict(&lck->data->share_modes[i],
>  				   access_mask, share_access)) {
> +
> +			if (share_mode_stale_pid(lck->data, i)) {
> +				continue;
> +			}
> +
>  			return NT_STATUS_SHARING_VIOLATION;
>  		}
>  	}
> -- 
> 1.7.7.3
> 
> 
> >From b22570b7ee007edbfcf2869e34c508f9f583cc85 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 May 2012 15:23:29 +0200
> Subject: [PATCH 04/14] s3: Check for serverid_exists in smb_posix_unlink
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/smbd/trans2.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/source3/smbd/trans2.c b/source3/smbd/trans2.c
> index 590ee5b..936b39a 100644
> --- a/source3/smbd/trans2.c
> +++ b/source3/smbd/trans2.c
> @@ -7595,6 +7595,9 @@ static NTSTATUS smb_posix_unlink(connection_struct *conn,
>  			if (e->flags & SHARE_MODE_FLAG_POSIX_OPEN) {
>  				continue;
>  			}
> +			if (share_mode_stale_pid(lck->data, i)) {
> +				continue;
> +			}
>  			/* Fail with sharing violation. */
>  			close_file(req, fsp, NORMAL_CLOSE);
>  			TALLOC_FREE(lck);
> -- 
> 1.7.7.3
> 
> 
> >From 8fcf75e9ccededc027cae081c9eec7beaa86cf44 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 May 2012 16:34:11 +0200
> Subject: [PATCH 05/14] s3: Do not check the PIDs is parse_share_modes
> 
> We do that when conflicts arise
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/locking/share_mode_lock.c |   42 -------------------------------------
>  1 files changed, 0 insertions(+), 42 deletions(-)
> 
> diff --git a/source3/locking/share_mode_lock.c b/source3/locking/share_mode_lock.c
> index 493bc15b..171c72f 100644
> --- a/source3/locking/share_mode_lock.c
> +++ b/source3/locking/share_mode_lock.c
> @@ -118,9 +118,6 @@ static struct share_mode_data *parse_share_modes(TALLOC_CTX *mem_ctx,
>  						 const TDB_DATA dbuf)
>  {
>  	struct share_mode_data *d;
> -	int i;
> -	struct server_id *pids;
> -	bool *pid_exists;
>  	enum ndr_err_code ndr_err;
>  	DATA_BLOB blob;
>  
> @@ -148,45 +145,6 @@ static struct share_mode_data *parse_share_modes(TALLOC_CTX *mem_ctx,
>  		NDR_PRINT_DEBUG(share_mode_data, d);
>  	}
>  
> -	/*
> -	 * Ensure that each entry has a real process attached.
> -	 */
> -
> -	pids = talloc_array(talloc_tos(), struct server_id,
> -			    d->num_share_modes);
> -	if (pids == NULL) {
> -		DEBUG(0, ("talloc failed\n"));
> -		goto fail;
> -	}
> -	pid_exists = talloc_array(talloc_tos(), bool, d->num_share_modes);
> -	if (pid_exists == NULL) {
> -		DEBUG(0, ("talloc failed\n"));
> -		goto fail;
> -	}
> -
> -	for (i=0; i<d->num_share_modes; i++) {
> -		pids[i] = d->share_modes[i].pid;
> -	}
> -	if (!serverids_exist(pids, d->num_share_modes, pid_exists)) {
> -		DEBUG(0, ("serverid_exists failed\n"));
> -		goto fail;
> -	}
> -
> -	i = 0;
> -	while (i < d->num_share_modes) {
> -		struct share_mode_entry *e = &d->share_modes[i];
> -		if (!pid_exists[i]) {
> -			DEBUG(10, ("wipe non-existent pid %s\n",
> -				   procid_str_static(&e->pid)));
> -			*e = d->share_modes[d->num_share_modes-1];
> -			d->num_share_modes -= 1;
> -			d->modified = True;
> -			continue;
> -		}
> -		i += 1;
> -	}
> -	TALLOC_FREE(pid_exists);
> -	TALLOC_FREE(pids);
>  	return d;
>  fail:
>  	TALLOC_FREE(d);
> -- 
> 1.7.7.3
> 
> 
> >From 8020308a17f316ca3c0f73704fdd56481397f3e7 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 May 2012 15:23:10 +0200
> Subject: [PATCH 06/14] s3: Check for serverid_exists in rename_share_filename
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/locking/locking.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/source3/locking/locking.c b/source3/locking/locking.c
> index df21104..69a6f26 100644
> --- a/source3/locking/locking.c
> +++ b/source3/locking/locking.c
> @@ -556,6 +556,10 @@ bool rename_share_filename(struct messaging_context *msg_ctx,
>  			continue;
>  		}
>  
> +		if (share_mode_stale_pid(d, i)) {
> +			continue;
> +		}
> +
>  		DEBUG(10,("rename_share_filename: sending rename message to "
>  			  "pid %s file_id %s sharepath %s base_name %s "
>  			  "stream_name %s\n",
> -- 
> 1.7.7.3
> 
> 
> >From 34eb4c596c3cf715ed57f4963363da496a6d57c0 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Fri, 11 May 2012 14:39:42 +0200
> Subject: [PATCH 07/14] s3: Test whether get_share_mode_lock cleans up stale
>  processes
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/Makefile.in            |    3 +-
>  source3/torture/proto.h        |    1 +
>  source3/torture/test_cleanup.c |  175 ++++++++++++++++++++++++++++++++++++++++
>  source3/torture/torture.c      |    1 +
>  source3/wscript_build          |    3 +-
>  5 files changed, 181 insertions(+), 2 deletions(-)
> 
> diff --git a/source3/Makefile.in b/source3/Makefile.in
> index 1eb8cd8..984cc00 100644
> --- a/source3/Makefile.in
> +++ b/source3/Makefile.in
> @@ -1282,11 +1282,12 @@ SMBTORTURE_OBJ1 = torture/torture.o torture/nbio.o torture/scanner.o torture/uta
>  		torture/t_strappend.o
>  
>  SMBTORTURE_OBJ = $(SMBTORTURE_OBJ1) $(PARAM_OBJ) $(TLDAP_OBJ) \
> -	$(LIBSMB_OBJ) $(KRBCLIENT_OBJ) $(LIB_NONSMBD_OBJ) \
> +	$(LIBSMB_OBJ) $(KRBCLIENT_OBJ) $(LIB_NONSMBD_OBJ) $(LOCKING_OBJ) \
>  	@LIBWBCLIENT_STATIC@ \
>          torture/wbc_async.o \
>          ../nsswitch/wb_reqtrans.o \
>  	../libcli/lsarpc/util_lsarpc.o \
> +	lib/filename_util.o \
>  	$(LIBMSRPC_OBJ) $(LIBMSRPC_GEN_OBJ) $(LIBCLI_ECHO_OBJ)
>  
>  MASKTEST_OBJ = torture/masktest.o $(PARAM_OBJ) $(LIBSMB_OBJ) $(KRBCLIENT_OBJ) \
> diff --git a/source3/torture/proto.h b/source3/torture/proto.h
> index 80618ce..4104fbc 100644
> --- a/source3/torture/proto.h
> +++ b/source3/torture/proto.h
> @@ -104,6 +104,7 @@ bool run_local_conv_auth_info(int dummy);
>  bool run_local_sprintf_append(int dummy);
>  bool run_cleanup1(int dummy);
>  bool run_cleanup2(int dummy);
> +bool run_cleanup3(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 39f579a..d9dce40 100644
> --- a/source3/torture/test_cleanup.c
> +++ b/source3/torture/test_cleanup.c
> @@ -18,11 +18,14 @@
>  */
>  
>  #include "includes.h"
> +#include "locking/proto.h"
>  #include "torture/proto.h"
>  #include "system/filesys.h"
> +#include "system/select.h"
>  #include "libsmb/libsmb.h"
>  #include "libcli/smb/smbXcli_base.h"
>  #include "libcli/security/security.h"
> +#include "librpc/gen_ndr/open_files.h"
>  
>  bool run_cleanup1(int dummy)
>  {
> @@ -154,3 +157,175 @@ bool run_cleanup2(int dummy)
>  	}
>  	return true;
>  }
> +
> +static bool create_stale_share_mode_entry(const char *fname,
> +					  struct file_id *p_id)
> +{
> +	struct cli_state *cli;
> +	uint16_t fnum;
> +	NTSTATUS status;
> +	SMB_STRUCT_STAT sbuf;
> +	struct file_id id;
> +
> +	if (!torture_open_connection(&cli, 0)) {
> +		return false;
> +	}
> +
> +	status = torture_setup_unix_extensions(cli);
> +	if (!NT_STATUS_IS_OK(status)) {
> +		printf("torture_setup_unix_extensions failed: %s\n",
> +		       nt_errstr(status));
> +		return false;
> +	}
> +	status = cli_openx(cli, fname, O_RDWR|O_CREAT, DENY_ALL, &fnum);
> +	if (!NT_STATUS_IS_OK(status)) {
> +		printf("open of %s failed (%s)\n", fname, nt_errstr(status));
> +		return false;
> +	}
> +	status = cli_posix_stat(cli, fname, &sbuf);
> +	if (!NT_STATUS_IS_OK(status)) {
> +		printf("cli_posix_stat failed: %s\n", nt_errstr(status));
> +		return false;
> +	}
> +	status = smbXcli_conn_samba_suicide(cli->conn, 1);
> +	if (!NT_STATUS_IS_OK(status)) {
> +		printf("smbXcli_conn_samba_suicide failed: %s\n",
> +		       nt_errstr(status));
> +		return false;
> +	}
> +
> +	id.devid = sbuf.st_ex_rdev;
> +	id.inode = sbuf.st_ex_ino;
> +	id.extid = 0;
> +
> +	poll(NULL, 0, 1000);
> +
> +	*p_id = id;
> +	return true;
> +}
> +
> +static bool corrupt_dummy(struct share_mode_data *d)
> +{
> +	return true;
> +}
> +
> +static bool invalidate_sharemode(struct share_mode_data *d)
> +{
> +	d->share_modes[0].op_type =
> +		OPLOCK_EXCLUSIVE|OPLOCK_BATCH|OPLOCK_LEVEL_II;
> +	d->modified = true;
> +	return true;
> +}
> +
> +static bool duplicate_entry(struct share_mode_data *d, int i)
> +{
> +	struct share_mode_entry *tmp;
> +
> +	if (i >= d->num_share_modes) {
> +		return false;
> +	}
> +
> +	tmp = talloc_realloc(d, d->share_modes, struct share_mode_entry,
> +			     d->num_share_modes + 1);
> +	if (tmp == NULL) {
> +		return false;
> +	}
> +	d->share_modes = tmp;
> +	d->num_share_modes += 1;
> +	d->share_modes[d->num_share_modes-1] = d->share_modes[i];
> +	d->modified = true;
> +	return true;
> +}
> +
> +static bool create_duplicate_batch(struct share_mode_data *d)
> +{
> +	if (d->num_share_modes != 1) {
> +		return false;
> +	}
> +	d->share_modes[0].op_type = OPLOCK_BATCH;
> +	if (!duplicate_entry(d, 0)) {
> +		return false;
> +	}
> +	return true;
> +}
> +
> +struct corruption_fns {
> +	bool (*fn)(struct share_mode_data *d);
> +	const char *descr;
> +};
> +
> +bool run_cleanup3(int dummy)
> +{
> +	struct cli_state *cli;
> +	const char *fname = "cleanup3";
> +	uint16_t fnum;
> +	NTSTATUS status;
> +	struct share_mode_lock *lck;
> +	struct file_id id;
> +	size_t i;
> +
> +	struct corruption_fns fns[] = {
> +		{ corrupt_dummy, "no corruption" },
> +		{ invalidate_sharemode, "invalidate_sharemode" },
> +		{ create_duplicate_batch, "create_duplicate_batch" },
> +	};
> +
> +	printf("CLEANUP3: Checking that a share mode is cleaned up on "
> +	       "conflict\n");
> +
> +	for (i=0; i<ARRAY_SIZE(fns); i++) {
> +
> +		printf("testing %s\n", fns[i].descr);
> +
> +		if (!create_stale_share_mode_entry(fname, &id)) {
> +			printf("create_stale_entry failed\n");
> +			return false;
> +		}
> +
> +		printf("%d %d %d\n", (int)id.devid, (int)id.inode,
> +		       (int)id.extid);
> +
> +		if (!locking_init()) {
> +			printf("locking_init failed\n");
> +			return false;
> +		}
> +		lck = get_existing_share_mode_lock(talloc_tos(), id);
> +		if (lck == NULL) {
> +			printf("get_existing_share_mode_lock failed\n");
> +			return false;
> +		}
> +		if (lck->data->num_share_modes != 1) {
> +			printf("get_existing_share_mode_lock did clean up\n");
> +			return false;
> +		}
> +
> +		fns[i].fn(lck->data);
> +
> +		TALLOC_FREE(lck);
> +
> +		if (!torture_open_connection(&cli, 0)) {
> +			return false;
> +		}
> +		status = cli_openx(cli, fname, O_RDWR|O_CREAT, DENY_ALL,
> +				   &fnum);
> +		if (!NT_STATUS_IS_OK(status)) {
> +			printf("open of %s failed (%s)\n", fname,
> +			       nt_errstr(status));
> +			return false;
> +		}
> +		lck = get_existing_share_mode_lock(talloc_tos(), id);
> +		if (lck == NULL) {
> +			printf("get_existing_share_mode_lock failed\n");
> +			return false;
> +		}
> +		if (lck->data->num_share_modes != 1) {
> +			printf("conflicting open did not clean up\n");
> +			return false;
> +		}
> +		TALLOC_FREE(lck);
> +
> +		torture_close_connection(cli);
> +	}
> +
> +	return true;
> +}
> diff --git a/source3/torture/torture.c b/source3/torture/torture.c
> index 83b0666..bee7667 100644
> --- a/source3/torture/torture.c
> +++ b/source3/torture/torture.c
> @@ -9019,6 +9019,7 @@ static struct {
>  	{ "SMB2-SESSION-REAUTH", run_smb2_session_reauth },
>  	{ "CLEANUP1", run_cleanup1 },
>  	{ "CLEANUP2", run_cleanup2 },
> +	{ "CLEANUP3", run_cleanup3 },
>  	{ "LOCAL-SUBSTITUTE", run_local_substitute, 0},
>  	{ "LOCAL-GENCACHE", run_local_gencache, 0},
>  	{ "LOCAL-TALLOC-DICT", run_local_talloc_dict, 0},
> diff --git a/source3/wscript_build b/source3/wscript_build
> index 4deb556..db31dbd 100755
> --- a/source3/wscript_build
> +++ b/source3/wscript_build
> @@ -552,7 +552,7 @@ SMBTORTURE_SRC1 = '''torture/torture.c torture/nbio.c torture/scanner.c torture/
>  		torture/test_idmap_tdb_common.c
>                  torture/t_strappend.c'''
>  
> -SMBTORTURE_SRC = '''${SMBTORTURE_SRC1}
> +SMBTORTURE_SRC = '''${SMBTORTURE_SRC1} ${LOCKING_SRC} ${FNAME_UTIL_SRC}
>          torture/wbc_async.c'''
>  
>  MASKTEST_SRC = '''torture/masktest.c'''
> @@ -1381,6 +1381,7 @@ bld.SAMBA3_BINARY('smbtorture' + bld.env.suffix3,
>                   TLDAP
>                   RPC_NDR_ECHO
>                   WB_REQTRANS
> +		 NDR_OPEN_FILES
>  		 idmap
>                   ''',
>                   vars=locals())
> -- 
> 1.7.7.3
> 
> 
> >From 34b6f8a2884702f46dd47370172d5088edc2c0de Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 May 2012 15:23:29 +0200
> Subject: [PATCH 08/14] s3: Check for serverid_exists in find_oplock_types
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/smbd/open.c |   18 ++++++++++++++++++
>  1 files changed, 18 insertions(+), 0 deletions(-)
> 
> diff --git a/source3/smbd/open.c b/source3/smbd/open.c
> index 8a5273e..774b5fc 100644
> --- a/source3/smbd/open.c
> +++ b/source3/smbd/open.c
> @@ -1128,6 +1128,10 @@ static void find_oplock_types(files_struct *fsp,
>  
>  		if (BATCH_OPLOCK_TYPE(lck->data->share_modes[i].op_type)) {
>  			/* batch - can only be one. */
> +			if (share_mode_stale_pid(lck->data, i)) {
> +				DEBUG(10, ("Found stale batch oplock\n"));
> +				continue;
> +			}
>  			if (*pp_ex_or_batch || *pp_batch || *got_level2 || *got_no_oplock) {
>  				smb_panic("Bad batch oplock entry.");
>  			}
> @@ -1135,6 +1139,10 @@ static void find_oplock_types(files_struct *fsp,
>  		}
>  
>  		if (EXCLUSIVE_OPLOCK_TYPE(lck->data->share_modes[i].op_type)) {
> +			if (share_mode_stale_pid(lck->data, i)) {
> +				DEBUG(10, ("Found stale duplicate oplock\n"));
> +				continue;
> +			}
>  			/* Exclusive or batch - can only be one. */
>  			if (*pp_ex_or_batch || *got_level2 || *got_no_oplock) {
>  				smb_panic("Bad exclusive or batch oplock entry.");
> @@ -1144,6 +1152,11 @@ static void find_oplock_types(files_struct *fsp,
>  
>  		if (LEVEL_II_OPLOCK_TYPE(lck->data->share_modes[i].op_type)) {
>  			if (*pp_batch || *pp_ex_or_batch) {
> +				if (share_mode_stale_pid(lck->data, i)) {
> +					DEBUG(10, ("Found stale LevelII "
> +						   "oplock\n"));
> +					continue;
> +				}
>  				smb_panic("Bad levelII oplock entry.");
>  			}
>  			*got_level2 = true;
> @@ -1151,6 +1164,11 @@ static void find_oplock_types(files_struct *fsp,
>  
>  		if (lck->data->share_modes[i].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 "
> +						   "entry\n"));
> +					continue;
> +				}
>  				smb_panic("Bad no oplock entry.");
>  			}
>  			*got_no_oplock = true;
> -- 
> 1.7.7.3
> 
> 
> >From fb433ecb7e6de471469c25ed1cd3a19a60a54420 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 14 May 2012 14:57:34 +0200
> Subject: [PATCH 09/14] s3: Be less picky on stale share mode entries
> 
> If a process died, the share mode entry might be bogus. Ignore those entries.
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/locking/locking.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/source3/locking/locking.c b/source3/locking/locking.c
> index 69a6f26..b9fba17 100644
> --- a/source3/locking/locking.c
> +++ b/source3/locking/locking.c
> @@ -620,7 +620,9 @@ bool is_valid_share_mode_entry(const struct share_mode_entry *e)
>  	num_props += (EXCLUSIVE_OPLOCK_TYPE(e->op_type) ? 1 : 0);
>  	num_props += (LEVEL_II_OPLOCK_TYPE(e->op_type) ? 1 : 0);
>  
> -	SMB_ASSERT(num_props <= 1);
> +	if (serverid_exists(&e->pid) && (num_props > 1)) {
> +		smb_panic("Invalid share mode entry");
> +	}
>  	return (num_props != 0);
>  }
>  
> -- 
> 1.7.7.3
> 
> 
> >From a03d82a0f5e9b56293a480fd9f6e0617956a7784 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 May 2012 15:23:29 +0200
> Subject: [PATCH 10/14] s3: Check for serverid_exists in
>  close_remove_share_mode
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/smbd/close.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/source3/smbd/close.c b/source3/smbd/close.c
> index 7ecda5c..9069849 100644
> --- a/source3/smbd/close.c
> +++ b/source3/smbd/close.c
> @@ -425,6 +425,9 @@ static NTSTATUS close_remove_share_mode(files_struct *fsp,
>  				if (fsp->posix_open && (e->flags & SHARE_MODE_FLAG_POSIX_OPEN)) {
>  					continue;
>  				}
> +				if (share_mode_stale_pid(lck->data, i)) {
> +					continue;
> +				}
>  				delete_file = False;
>  				break;
>  			}
> -- 
> 1.7.7.3
> 
> 
> >From 0be3837397010469bd4f9fae5c29ecad3a8974b2 Mon Sep 17 00:00:00 2001
> From: Volker Lendecke <vl at samba.org>
> Date: Mon, 7 May 2012 15:23:29 +0200
> Subject: [PATCH 11/14] s3: Check for serverid_exists in close_directory
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/smbd/close.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/source3/smbd/close.c b/source3/smbd/close.c
> index 9069849..cffd344 100644
> --- a/source3/smbd/close.c
> +++ b/source3/smbd/close.c
> @@ -1090,6 +1090,9 @@ static NTSTATUS close_directory(struct smb_request *req, files_struct *fsp,
>  				if (fsp->posix_open && (e->flags & SHARE_MODE_FLAG_POSIX_OPEN)) {
>  					continue;
>  				}
> +				if (share_mode_stale_pid(lck->data, i)) {
> +					continue;
> +				}
>  				delete_dir = False;
>  				break;
>  			}
> -- 
> 1.7.7.3
> 
> 
> >From 8b2b8f298c2fdd63dec0d01b4fae359860fb443d Mon Sep 17 00:00:00 2001
> From: Stefan Metzmacher <metze at samba.org>
> Date: Wed, 16 May 2012 09:11:40 +0200
> Subject: [PATCH 12/14] s3:selftest: run smbtorture3 CLEANUP3 in the
>  s3dc:local environment
> 
> metze
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> ---
>  source3/selftest/tests.py |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index fa1f5e5..ce80274 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -90,6 +90,10 @@ for t in tests:
>      plantestsuite("samba3.smbtorture_s3.crypt(s3dc).%s" % t, "s3dc", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/tmp', '$USERNAME', '$PASSWORD', binpath('smbtorture3'), "-e", "-l $LOCAL_PATH"])
>      plantestsuite("samba3.smbtorture_s3.plain(dc).%s" % t, "dc", [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/tmp', '$USERNAME', '$PASSWORD', binpath('smbtorture3'), "", "-l $LOCAL_PATH"])
>  
> +env = "s3dc:local"
> +t = "CLEANUP3"
> +plantestsuite("samba3.smbtorture_s3.plain(%s).%s" % (env, t), env, [os.path.join(samba3srcdir, "script/tests/test_smbtorture_s3.sh"), t, '//$SERVER_IP/tmp', '$USERNAME', '$PASSWORD', binpath('smbtorture3'), "", "-l $LOCAL_PATH"])
> +
>  local_tests=[
>  	"LOCAL-SUBSTITUTE",
>  	"LOCAL-GENCACHE",
> -- 
> 1.7.7.3
> 
> 
> >From 9b6a71d20f64519eead1eca6f7fe5ce188259c63 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Tue, 22 May 2012 12:27:06 -0700
> Subject: [PATCH 13/14] Fix an invalid state only reachable on server
>  crash/abort.
> 
> Remove any delete-on-close tokens and clear the count if there are no
> valid share modes.
> ---
>  source3/locking/locking.c |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
> 
> diff --git a/source3/locking/locking.c b/source3/locking/locking.c
> index b9fba17..2d542ca 100644
> --- a/source3/locking/locking.c
> +++ b/source3/locking/locking.c
> @@ -659,6 +659,16 @@ bool share_mode_stale_pid(struct share_mode_data *d, unsigned i)
>  		   (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) {
> +		/* We cannot have any delete
> +		   tokens if there are no valid
> +		   share modes. */
> +		TALLOC_FREE(d->delete_tokens);
> +		d->num_delete_tokens = 0;
> +	}
> +
>  	d->modified = true;
>  	return true;
>  }
> -- 
> 1.7.7.3
> 
> 
> >From 73b92625de0eada31179cd7332c917a764bec3d4 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Tue, 22 May 2012 12:28:04 -0700
> Subject: [PATCH 14/14] Ensure we only return NT_STATUS_DELETE_PENDING if the
>  share modes are valid.
> 
> Ensure we only return *file_existed = true if there were valid share modes.
> ---
>  source3/smbd/open.c |   21 ++++++++++++++++++---
>  1 files changed, 18 insertions(+), 3 deletions(-)
> 
> diff --git a/source3/smbd/open.c b/source3/smbd/open.c
> index 774b5fc..c3e0ded 100644
> --- a/source3/smbd/open.c
> +++ b/source3/smbd/open.c
> @@ -984,12 +984,21 @@ static NTSTATUS open_mode_check(connection_struct *conn,
>  		return NT_STATUS_OK;
>  	}
>  
> -	*file_existed = True;
> -
>  	/* A delete on close prohibits everything */
>  
>  	if (is_delete_on_close_set(lck, name_hash)) {
> -		return NT_STATUS_DELETE_PENDING;
> +		/* Check the delete on close token
> +		   is valid. It could have been left
> +		   after a server crash. */
> +		for(i = 0; i < lck->data->num_share_modes; i++) {
> +			if (!share_mode_stale_pid(lck->data, i)) {
> +
> +				*file_existed = true;
> +
> +				return NT_STATUS_DELETE_PENDING;
> +			}
> +		}
> +		return NT_STATUS_OK;
>  	}
>  
>  	if (is_stat_open(access_mask)) {
> @@ -1029,10 +1038,16 @@ static NTSTATUS open_mode_check(connection_struct *conn,
>  				continue;
>  			}
>  
> +			*file_existed = true;
> +
>  			return NT_STATUS_SHARING_VIOLATION;
>  		}
>  	}
>  
> +	if (lck->data->num_share_modes != 0) {
> +		*file_existed = true;
> +	}
> +
>  	return NT_STATUS_OK;
>  }
>  
> -- 
> 1.7.7.3
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 206 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20120524/c9462fea/attachment.pgp>


More information about the samba-technical mailing list