Questions about smbd option "strict rename"

[Jeremy Allison]

On Tue, Nov 24, 2015 at 03:54:55PM +0100, Ralph Boehme wrote:
> Hi Michael!
> 
> On Tue, Nov 24, 2015 at 12:55:43PM +0100, Michael Adam wrote:
> > LGTM.
> > 
> > Attached is a version of Jeremy's patch with a minor
> > change (use struct initializers).
> 
> looks like the main state was still missing an initializer, so struct
> have_file_open_below_state.found_one was never initialized to false.
> 
> Updated patch attached.

Thanks for catching that ! Reviewed-by: Jeremy Allison <jra at samba.org>

> -- 
> 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@sernet.de

> From 0f89dedbbf9260104111895c6e5cb95a28c27438 Mon Sep 17 00:00:00 2001
> From: Jeremy Allison <jra at samba.org>
> Date: Mon, 23 Nov 2015 14:00:56 -0800
> Subject: [PATCH] s3: smbd: have_file_open_below() fails to enumerate open
>  files below an open directory handle.
> 
> There are three issues:
> 
> 1). The memcmp checking that the open file path has the open
> directory path as its parent compares using the wrong length
> (it uses the full open file path which will never compare as
> the same).
> 
> 2). The files_below_forall() function doesn't fill in the
> callback function or callback data when calling share_mode_forall(),
> leading to a crash (which we never saw, as the previous issue (1)
> meant the callback function would never be invoked).
> 
> 3). When invoking the callback function from files_below_forall_fn()
> we were passing in the wrong private_data pointer (needs to be
> the one from the state, not the private_data passed into
> files_below_forall_fn()).
> 
> Found when running the torture test smb2.rename.rename_dir_openfile
> when fixing bug #11065.
> 
> BUG: https://bugzilla.samba.org/show_bug.cgi?id=11615
> 
> Signed-off-by: Jeremy Allison <jra at samba.org>
> Reviewed-by: Michael Adam <obnox at samba.org>
> Reviewed-by: Ralph Boehme <slow at samba.org>
> ---
>  source3/smbd/dir.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
> index 3f99f88..82721c1 100644
> --- a/source3/smbd/dir.c
> +++ b/source3/smbd/dir.c
> @@ -1912,14 +1912,14 @@ static int files_below_forall_fn(struct file_id fid,
>  		return 0;
>  	}
>  
> -	if (memcmp(state->dirpath, fullpath, len) != 0) {
> +	if (memcmp(state->dirpath, fullpath, state->dirpath_len) != 0) {
>  		/*
>  		 * Not a parent
>  		 */
>  		return 0;
>  	}
>  
> -	return state->fn(fid, data, private_data);
> +	return state->fn(fid, data, state->private_data);
>  }
>  
>  static int files_below_forall(connection_struct *conn,
> @@ -1929,7 +1929,10 @@ static int files_below_forall(connection_struct *conn,
>  					void *private_data),
>  			      void *private_data)
>  {
> -	struct files_below_forall_state state = {};
> +	struct files_below_forall_state state = {
> +			.fn = fn,
> +			.private_data = private_data,
> +	};
>  	int ret;
>  	char tmpbuf[PATH_MAX];
>  	char *to_free;
> @@ -1964,7 +1967,9 @@ static int have_file_open_below_fn(struct file_id fid,
>  static bool have_file_open_below(connection_struct *conn,
>  				 const struct smb_filename *name)
>  {
> -	struct have_file_open_below_state state = {};
> +	struct have_file_open_below_state state = {
> +		.found_one = false,
> +	};
>  	int ret;
>  
>  	if (!VALID_STAT(name->st)) {
> -- 
> 2.5.0
>