Questions about smbd option "strict rename"

[Ralph Boehme]

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.

-Ralph

-- 
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
-------------- next part --------------
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