[PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

Samba - samba-technical mailing list
smbd coredumps when a chdir call fails during the server shutdown. This
should be avoided as the failing call is already logged and the coredump
does not provide additional information.

Christof

patches (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

Samba - samba-technical mailing list
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!

> +       "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).

Thanks,

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

Samba - samba-technical mailing list
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.

Christof

patches (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

Samba - samba-technical mailing list
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.

Thanks,

Andrew Bartlett

--
Andrew Bartlett
https://samba.org/~abartlet/
Authentication Developer, Samba Team         https://samba.org
Samba Development and Support, Catalyst IT  
https://catalyst.net.nz/services/samba





Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

Samba - samba-technical mailing list
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

patches (12K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

Samba - samba-technical mailing list
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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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 <[hidden email]>
> 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 <[hidden email]>
> ---
>  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
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

Samba - samba-technical mailing list
On Fri, 2017-12-15 at 11:24 -0800, Jeremy Allison wrote:

> 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 !
I likewise reviewed and pushed it.  However autobuild fails, it needs
the attached fixup.  (I had meant to push it again with this folded in,
but clearly did not).

Andrew Bartlett
--
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

0001-fixup-missing-static-decl.patch (1K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

Samba - samba-technical mailing list
On Sat, Dec 16, 2017 at 09:14:39AM +1300, Andrew Bartlett via samba-technical wrote:

> On Fri, 2017-12-15 at 11:24 -0800, Jeremy Allison wrote:
> > 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 !
>
> I likewise reviewed and pushed it.  However autobuild fails, it needs
> the attached fixup.  (I had meant to push it again with this folded in,
> but clearly did not).

Thanks Andrew, will squash and re-push.

> --
> Andrew Bartlett                       http://samba.org/~abartlet/
> Authentication Developer, Samba Team  http://samba.org
> Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba

> From 23b42e4ce852b34db28964012bf8ed7535fede6a Mon Sep 17 00:00:00 2001
> From: Andrew Bartlett <[hidden email]>
> Date: Fri, 15 Dec 2017 11:14:29 +1300
> Subject: [PATCH] fixup missing static decl
>
> Signed-off-by: Andrew Bartlett <[hidden email]>
> ---
>  source3/modules/vfs_error_inject.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/source3/modules/vfs_error_inject.c b/source3/modules/vfs_error_inject.c
> index e77da4d8310..bb5477a449f 100644
> --- a/source3/modules/vfs_error_inject.c
> +++ b/source3/modules/vfs_error_inject.c
> @@ -92,6 +92,7 @@ static struct vfs_fn_pointers vfs_error_inject_fns = {
>   .chdir_fn = vfs_error_inject_chdir,
>  };
>  
> +static_decl_vfs;
>  NTSTATUS vfs_error_inject_init(TALLOC_CTX *ctx)
>  {
>   return smb_register_vfs(SMB_VFS_INTERFACE_VERSION, "error_inject",
> --
> 2.11.0
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCHES][BUG 13189] Fix coredump on failing chdir during logoff

Samba - samba-technical mailing list
On Fri, Dec 15, 2017 at 12:38:43PM -0800, Jeremy Allison wrote:

> On Sat, Dec 16, 2017 at 09:14:39AM +1300, Andrew Bartlett via samba-technical wrote:
> > On Fri, 2017-12-15 at 11:24 -0800, Jeremy Allison wrote:
> > > 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 !
> >
> > I likewise reviewed and pushed it.  However autobuild fails, it needs
> > the attached fixup.  (I had meant to push it again with this folded in,
> > but clearly did not).
>
> Thanks Andrew, will squash and re-push.

Thank you both. It might make sense to use static_decl_vfs in all vfs
modules for consistency and to provide the correct example for anybody
looking into existing code to start a new module. I can look into
writing a patch for this.

Christof