[PATCHES][BUG 13189] Fix coredump on failing chdir during logoff
Jeremy Allison
jra at samba.org
Fri Dec 15 19:24:53 UTC 2017
On Thu, Dec 14, 2017 at 01:06:13PM -0700, Christof Schmitt via samba-technical wrote:
> 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.
Nice work Christof, thanks. RB+ and pushed !
> Christof
> 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