Questions about smbd option "strict rename"

[Michael Adam]

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.
-------------- next part --------------
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/64367a9d/signature.sig>