[PATCHES][BUG 13189] Fix coredump on failing chdir during logoff
Christof Schmitt
cs at samba.org
Thu Dec 14 20:06:13 UTC 2017
On Thu, Dec 14, 2017 at 12:02:18PM +1300, Andrew Bartlett wrote:
> On Wed, 2017-12-13 at 15:50 -0700, Christof Schmitt wrote:
> > On Thu, Dec 14, 2017 at 11:07:00AM +1300, Andrew Bartlett wrote:
> > > On Wed, 2017-12-13 at 13:24 -0700, Christof Schmitt via samba-technical
> > > wrote:
> > >
> > > This looks great! I love that you went to such effort to create an
> > > automated test for this!
> >
> > Thanks. I wanted an easy way to trigger this problem, and it might be
> > useful in general to trigger errors for testing. Wrapping that in test
> > script was then a quick addition.
> >
> > > > + "SERVERCONFFILE",
> > >
> > > This is the only part I don't think you need, you can use SMB_CONF_FILE
> > > for this as you are running with :local (that is essentially what
> > > :local does).
> >
> > Correct, i missed that part. Updated patches are attached.
>
> I think we should we check that a panic would be detected. I realise
> that is a real pain, but otherwise I fear a change to the logging would
> render the patch neutered.
>
> Just make the fault injection module also call panic directly.
>
> Then, finally, put the test first, with a knownfail, and then remove
> the knownfail with the fix.
>
> To be clear, this is great, but if you can do this one last change I
> would appreciate it.
See attached patches for these changes.
Christof
-------------- next part --------------
From b9a153a10daee9acab3c8e1d6f456ad2f5fad28c Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 8 Dec 2017 15:29:07 -0700
Subject: [PATCH 1/5] vfs_error_inject: Add new module
This module allow injecting errors in vfs calls. It only implements one
case (return ESTALE from chdir), but the idea is to extend this to more
vfs functions and more errors when needed.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13189
Signed-off-by: Christof Schmitt <cs at samba.org>
---
source3/modules/vfs_error_inject.c | 99 ++++++++++++++++++++++++++++++++++++++
source3/modules/wscript_build | 7 +++
source3/wscript | 1 +
3 files changed, 107 insertions(+)
create mode 100644 source3/modules/vfs_error_inject.c
diff --git a/source3/modules/vfs_error_inject.c b/source3/modules/vfs_error_inject.c
new file mode 100644
index 0000000..e77da4d
--- /dev/null
+++ b/source3/modules/vfs_error_inject.c
@@ -0,0 +1,99 @@
+/*
+ * Unix SMB/CIFS implementation.
+ * Samba VFS module for error injection in VFS calls
+ * Copyright (C) Christof Schmitt 2017
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "includes.h"
+#include "smbd/smbd.h"
+
+#undef DBGC_CLASS
+#define DBGC_CLASS DBGC_VFS
+
+struct unix_error_map {
+ const char *err_str;
+ int error;
+} unix_error_map_array[] = {
+ { "ESTALE", ESTALE },
+};
+
+static int find_unix_error_from_string(const char *err_str)
+{
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(unix_error_map_array); i++) {
+ struct unix_error_map *m = &unix_error_map_array[i];
+
+ if (strequal(err_str, m->err_str)) {
+ return m->error;
+ }
+ }
+
+ return 0;
+}
+
+static int inject_unix_error(const char *vfs_func, vfs_handle_struct *handle)
+{
+ const char *err_str;
+
+ err_str = lp_parm_const_string(SNUM(handle->conn),
+ "error_inject", vfs_func, NULL);
+
+ if (err_str != NULL) {
+ int error;
+
+ error = find_unix_error_from_string(err_str);
+ if (error != 0) {
+ DBG_WARNING("Returning error %s for VFS function %s\n",
+ err_str, vfs_func);
+ return error;
+ }
+
+ if (strequal(err_str, "panic")) {
+ DBG_ERR("Panic in VFS function %s\n", vfs_func);
+ smb_panic("error_inject");
+ }
+
+ DBG_ERR("Unknown error inject %s requested "
+ "for vfs function %s\n", err_str, vfs_func);
+ }
+
+ return 0;
+}
+
+static int vfs_error_inject_chdir(vfs_handle_struct *handle,
+ const struct smb_filename *smb_fname)
+{
+ int error;
+
+ error = inject_unix_error("chdir", handle);
+ if (error != 0) {
+ errno = error;
+ return -1;
+ }
+
+ return SMB_VFS_NEXT_CHDIR(handle, smb_fname);
+}
+
+static struct vfs_fn_pointers vfs_error_inject_fns = {
+ .chdir_fn = vfs_error_inject_chdir,
+};
+
+NTSTATUS vfs_error_inject_init(TALLOC_CTX *ctx)
+{
+ return smb_register_vfs(SMB_VFS_INTERFACE_VERSION, "error_inject",
+ &vfs_error_inject_fns);
+}
diff --git a/source3/modules/wscript_build b/source3/modules/wscript_build
index 4e4fdb9..079302c 100644
--- a/source3/modules/wscript_build
+++ b/source3/modules/wscript_build
@@ -525,3 +525,10 @@ bld.SAMBA3_MODULE('vfs_fake_dfq',
init_function='',
internal_module=bld.SAMBA3_IS_STATIC_MODULE('vfs_fake_dfq'),
enabled=bld.SAMBA3_IS_ENABLED_MODULE('vfs_fake_dfq'))
+
+bld.SAMBA3_MODULE('vfs_error_inject',
+ subsystem='vfs',
+ source='vfs_error_inject.c',
+ init_function='',
+ internal_module=bld.SAMBA3_IS_STATIC_MODULE('vfs_error_inject'),
+ enabled=bld.SAMBA3_IS_ENABLED_MODULE('vfs_error_inject'))
diff --git a/source3/wscript b/source3/wscript
index d45a7d9..0f8fe54 100644
--- a/source3/wscript
+++ b/source3/wscript
@@ -1674,6 +1674,7 @@ main() {
if Options.options.enable_selftest or Options.options.developer:
default_shared_modules.extend(TO_LIST('vfs_fake_acls vfs_nfs4acl_xattr'))
+ default_shared_modules.extend(TO_LIST('vfs_error_inject'))
if conf.CONFIG_SET('AD_DC_BUILD_IS_ENABLED'):
default_static_modules.extend(TO_LIST('pdb_samba_dsdb auth_samba4 vfs_dfs_samba4'))
--
1.8.3.1
From f8b17fac4298a1e3a470fc352ba2cf41cc5b98b4 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 13 Dec 2017 11:34:05 -0700
Subject: [PATCH 2/5] selftest: Add share for error injection testing
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13189
Signed-off-by: Christof Schmitt <cs at samba.org>
---
selftest/target/Samba3.pm | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index afbc795..229435d 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -2150,6 +2150,10 @@ sub provision($$$$$$$$$)
[compound_find]
copy = tmp
smbd:find async delay usec = 10000
+[error_inject]
+ copy = tmp
+ vfs objects = error_inject
+ include = $libdir/error_inject.conf
";
close(CONF);
--
1.8.3.1
From c10a66e0f27368da79f16432004b39632eef13ab Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 13 Dec 2017 12:47:31 -0700
Subject: [PATCH 3/5] selftest: Make location of log file available in tests
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13189
Signed-off-by: Christof Schmitt <cs at samba.org>
---
selftest/selftest.pl | 1 +
1 file changed, 1 insertion(+)
diff --git a/selftest/selftest.pl b/selftest/selftest.pl
index e16696a..8c41459 100755
--- a/selftest/selftest.pl
+++ b/selftest/selftest.pl
@@ -877,6 +877,7 @@ my @exported_envvars = (
"RESOLV_CONF",
"UNACCEPTABLE_PASSWORD",
"LOCK_DIR",
+ "SMBD_TEST_LOG",
# nss_wrapper
"NSS_WRAPPER_PASSWD",
--
1.8.3.1
From e3f9d8045daf5cf7f100215da12f0f8e818df612 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 13 Dec 2017 12:58:18 -0700
Subject: [PATCH 4/5] selftest: Add test for failing chdir call in smbd
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13189
Signed-off-by: Christof Schmitt <cs at samba.org>
---
selftest/knownfail | 1 +
source3/script/tests/test_smbd_error.sh | 56 +++++++++++++++++++++++++++++++++
source3/selftest/tests.py | 3 ++
3 files changed, 60 insertions(+)
create mode 100755 source3/script/tests/test_smbd_error.sh
diff --git a/selftest/knownfail b/selftest/knownfail
index a28329c..f8bf283 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -347,3 +347,4 @@
# Disabling NTLM means you can't use samr to change the password
^samba.tests.ntlmdisabled.python\(ktest\).ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\)
^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
+^samba3.blackbox.smbd_error.check_panic_2
diff --git a/source3/script/tests/test_smbd_error.sh b/source3/script/tests/test_smbd_error.sh
new file mode 100755
index 0000000..e9af47a
--- /dev/null
+++ b/source3/script/tests/test_smbd_error.sh
@@ -0,0 +1,56 @@
+#!/bin/sh
+#
+# Test smbd with failing chdir system call.
+#
+# Verify that smbd does not panic when the chdir system call is
+# returning an error. ensure that the output format for ACL entries
+#
+# Copyright (C) 2017 Christof Schmitt
+
+. $(dirname $0)/../../../testprogs/blackbox/subunit.sh
+failed=0
+error_inject_conf=$(dirname $SMB_CONF_PATH)/error_inject.conf
+
+panic_count_0=$(grep -c PANIC $SMBD_TEST_LOG)
+
+#
+# Verify that a panic in smbd will result in a PANIC message in the log
+#
+
+# As a panic is expected here, also overwrite the default "panic
+# action" in selftest to not start a debugger
+echo 'error_inject:chdir = panic' > $error_inject_conf
+echo '[global]' >> $error_inject_conf
+echo 'panic action = ""' >> $error_inject_conf
+
+testit_expect_failure "smbclient" $VALGRIND \
+ $BINDIR/smbclient //$SERVER_IP/error_inject \
+ -U$USERNAME%$PASSWORD -c dir ||
+ failed=$(expr $failed + 1)
+
+rm $error_inject_conf
+
+panic_count_1=$(grep -c PANIC $SMBD_TEST_LOG)
+
+testit "check_panic_1" test $(expr $panic_count_0 + 1) -eq $panic_count_1 ||
+ failed=$(expr $failed + 1)
+
+#
+# Verify that a failing chdir vfs call does not result in a smbd panic
+#
+
+echo 'error_inject:chdir = ESTALE' > $error_inject_conf
+
+testit_expect_failure "smbclient" $VALGRIND \
+ $BINDIR/smbclient //$SERVER_IP/error_inject \
+ -U$USERNAME%$PASSWORD -c dir ||
+ failed=$(expr $failed + 1)
+
+panic_count_2=$(grep -c PANIC $SMBD_TEST_LOG)
+
+testit "check_panic_2" test $panic_count_1 -eq $panic_count_2 ||
+ failed=$(expr $failed + 1)
+
+rm $error_inject_conf
+
+testok $0 $failed
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index 1b35768..f6b42d6 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -584,6 +584,9 @@ plantestsuite("samba3.blackbox.net_tdb", "simpleserver:local",
smbclient3, '$SERVER', 'tmp', '$USERNAME', '$PASSWORD',
configuration, '$LOCAL_PATH', '$LOCK_DIR' ])
+plantestsuite("samba3.blackbox.smbd_error", "simpleserver:local",
+ [ os.path.join(samba3srcdir, "script/tests/test_smbd_error.sh") ])
+
plantestsuite("samba3.blackbox.net_cache_samlogon", "ad_member:local",
[ os.path.join(samba3srcdir, "script/tests/test_net_cache_samlogon.sh"),
'$SERVER', 'tmp', '$DC_USERNAME', '$DC_PASSWORD'])
--
1.8.3.1
From c7c13d04cf8b4fd9d707127ec91b039eb1f1d451 Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Wed, 13 Dec 2017 11:34:23 -0700
Subject: [PATCH 5/5] smbd: Fix coredump on failing chdir during logoff
server_exit does an internal tree disconnect which requires a chdir to
the share directory. In case the file system encountered a problem and
the chdir call returns an error, this triggers a SERVER_EXIT_ABNORMAL
which in turn results in a panic and a coredump. As the log already
indicates the problem (chdir returned an error), avoid the
SERVER_EXIT_ABNORMAL in this case and not trigger a coredump.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13189
Signed-off-by: Christof Schmitt <cs at samba.org>
---
selftest/knownfail | 1 -
source3/smbd/server_exit.c | 4 ----
2 files changed, 5 deletions(-)
diff --git a/selftest/knownfail b/selftest/knownfail
index f8bf283..a28329c 100644
--- a/selftest/knownfail
+++ b/selftest/knownfail
@@ -347,4 +347,3 @@
# Disabling NTLM means you can't use samr to change the password
^samba.tests.ntlmdisabled.python\(ktest\).ntlmdisabled.NtlmDisabledTests.test_samr_change_password\(ktest\)
^samba.tests.ntlmdisabled.python\(ad_dc_no_ntlm\).ntlmdisabled.NtlmDisabledTests.test_ntlm_connection\(ad_dc_no_ntlm\)
-^samba3.blackbox.smbd_error.check_panic_2
diff --git a/source3/smbd/server_exit.c b/source3/smbd/server_exit.c
index a9ef37f..dbeb247 100644
--- a/source3/smbd/server_exit.c
+++ b/source3/smbd/server_exit.c
@@ -150,8 +150,6 @@ static void exit_server_common(enum server_exit_reason how,
DEBUG(0, ("exit_server_common: "
"smb1srv_tcon_disconnect_all() failed (%s) - "
"triggering cleanup\n", nt_errstr(status)));
- how = SERVER_EXIT_ABNORMAL;
- reason = "smb1srv_tcon_disconnect_all failed";
}
status = smbXsrv_session_logoff_all(xconn);
@@ -161,8 +159,6 @@ static void exit_server_common(enum server_exit_reason how,
DEBUG(0, ("exit_server_common: "
"smbXsrv_session_logoff_all() failed (%s) - "
"triggering cleanup\n", nt_errstr(status)));
- how = SERVER_EXIT_ABNORMAL;
- reason = "smbXsrv_session_logoff_all failed";
}
}
--
1.8.3.1
More information about the samba-technical
mailing list