Questions about smbd option "strict rename"

[Michael Adam]

Oops, Ralph just pointed out to me that
a previous patch of yours in this thread
already contains this fix.
I will just push the whitspace fix then.

Cheers - Michael

On 2015-11-24 at 12:06 +0100, Michael Adam wrote:
> Looking through the code for have_file_open_below() triggered
> by this thread, I found that I think there is something missing
> (have_file_open_below_fn and the state not handed down through
> the traverse function to the final callback).
> 
> Attached patches should fix that.
> Volker has alreay given me rb+ on a private channel.
> Currently running a selftest with that.
> 
> Cheers - Michael
> 
> On 2015-11-23 at 14:16 -0800, Jeremy Allison wrote:
> > On Mon, Nov 23, 2015 at 01:33:44PM -0800, Jeremy Allison wrote:
> > > On Mon, Nov 23, 2015 at 10:22:35PM +0100, Ralph Boehme wrote:
> > > > On Mon, Nov 23, 2015 at 12:59:35PM +0100, Stefan Metzmacher wrote:
> > > > > Am 22.11.2015 um 13:49 schrieb Ralph Boehme:
> > > > > > On Fri, Nov 20, 2015 at 01:45:08PM -0800, Jeremy Allison wrote:
> > > > > >> On Fri, Nov 20, 2015 at 10:01:46AM +0100, Ralph Boehme wrote:
> > > > > >>> - "strict rename = no": doesn't work, opens are always checked,
> > > > > >>>   regardless of the setting of "strict rename".  can_rename(), the
> > > > > >>>   function where we do this check when renaming a directory, is
> > > > > >>>   missing a check for lp_strict_rename() or similar.
> > > > > >>
> > > > > >> Yep. That's how it was supposed to work. The code in
> > > > > >> can_rename() should probably be the same as the code in
> > > > > >> source/smbd/dir.c which is:
> > > > > >>
> > > > > >>         if (!lp_posix_pathnames() &&
> > > > > >>             lp_strict_rename(SNUM(conn)) &&
> > > > > >>             have_file_open_below(fsp->conn, fsp->fsp_name))
> > > > > >>         {
> > > > > >>                 return NT_STATUS_ACCESS_DENIED;
> > > > > >>         }
> > > > > > 
> > > > > > Ok, thanks for clarifying. Does everybody agree? Metze?
> > > > > 
> > > > > I don't agree, sorry.
> > > > > 
> > > > > We should provide the semantics the client asked for
> > > > > and try to behave like a windows server.
> > > > > 
> > > > > I think we should fix the manpage and use file_find_subpath()
> > > > > if lp_strict_rename() is false.
> > > > 
> > > > I'm leaning towards that interpretation of "strict rename" too.
> > > 
> > > I have a patch in preparation that implements this. It's
> > > found an existing bug in files_below_forall() I'll probably
> > > log as a separate bug....
> > 
> > And here is the patch that implements this interpretation of "strict rename"
> > and updates the docs.
> > 
> > Note it depends on the previous patch for bug:
> > 
> > https://bugzilla.samba.org/show_bug.cgi?id=11615
> > 
> > Bug 11615 - have_file_open_below() fails to enumerate files open under a directory correctly.
> > 
> > in order to pass the tests and enumerate open
> > files correctly.
> > 
> > Please review !
> > 
> > Thanks,
> > 
> > 	Jeremy.

> From 3df05d7aaeb823ec8d28dd4575d698391e0f5476 Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Tue, 24 Nov 2015 11:30:05 +0100
> Subject: [PATCH 1/2] smbd:dir: remove an extra empty line in
>  files_below_forall()
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> Reviewed-by: Volker Lendecke <vl at samba.org>
> ---
>  source3/smbd/dir.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
> index 3f99f88..0972340 100644
> --- a/source3/smbd/dir.c
> +++ b/source3/smbd/dir.c
> @@ -1940,7 +1940,6 @@ static int files_below_forall(connection_struct *conn,
>  					  &state.dirpath, &to_free);
>  	if (state.dirpath_len == -1) {
>  		return -1;
> -
>  	}
>  
>  	ret = share_mode_forall(files_below_forall_fn, &state);
> -- 
> 2.5.0
> 
> 
> From 59f536bc78942931aba870c3bc4e5c553a3c22a0 Mon Sep 17 00:00:00 2001
> From: Michael Adam <obnox at samba.org>
> Date: Tue, 24 Nov 2015 11:35:19 +0100
> Subject: [PATCH 2/2] smbd:dir: properly fill state with fn and private_data in
>  files_below_forall()
> 
> Signed-off-by: Michael Adam <obnox at samba.org>
> Reviewed-by: Volker Lendecke <vl at samba.org>
> ---
>  source3/smbd/dir.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/source3/smbd/dir.c b/source3/smbd/dir.c
> index 0972340..b9fff96 100644
> --- a/source3/smbd/dir.c
> +++ b/source3/smbd/dir.c
> @@ -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;
> -- 
> 2.5.0
> 



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151124/00e5d904/signature.sig>