Re: [PATCH]: core files with make testenv

[Michael Adam]

Hi,

I think these are good.

(i.e. "Reviewed-by: Michael Adam <ob...@samba.org>")

Anyone else?

I have analyzed the use of dump_core_setup() some more.
And I think we can do much better in the daemon start code.

Attached find a patchset that makes smbd,nmbd, and winbindd
all behave the same, so that they call dump_core_setup() four
times during startup to adapt to potential changes of the
logfile base:

- right at the start
- after parsing the command line
- after initial config loading (without registry)
- after loading the full config
(Or should we rather re-setup the core as part of the functions
reload_services/reload_nmbd_services/reload_services_file ?)

I still think Gregor's patch to hand in the logdir to the daemons via
command line in test setup is good, so that we are able to dump
core as early as possible.

Comments or push welcome.

Cheers - Michael

On 2013-04-22 at 14:18 +0200, Gregor Beck wrote:
> Hi,
>
> with make testenv SELFTEST_TESTENV=s3dc calls to dump_core_setup() usually
> fail so no core files where generated on failure.
>
>
> best regards,
> Gregor
>

> >From 30785c42a03e851061f4a332999a0c1ea6c40d99 Mon Sep 17 00:00:00 2001
> From: Gregor Beck <gb...@sernet.de>
> Date: Mon, 22 Apr 2013 12:35:01 +0200
> Subject: [PATCH 1/2] selftest:Samba3: pass -l logdir to daemon processes.
>
> The daemons try to set up the corefile path within logdir before the config
> file was read.
> ---
> selftest/target/Samba3.pm | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
> index 85ef64b..6a5c9a0 100755
> --- a/selftest/target/Samba3.pm
> +++ b/selftest/target/Samba3.pm
> @@ -653,7 +653,8 @@ sub check_or_start($$$$$) {
> close($env_vars->{STDIN_PIPE});
> open STDIN, ">&", \*STDIN_READER or die "can't dup STDIN_READER to STDIN: $!";
>
> - exec(@preargs, Samba::bindir_path($self, "nmbd"), "-F", "--no-process-group", "--log-stdout", "-s", $env_vars->{SERVERCONFFILE}, @optargs) or die("Unable to start nmbd: $!");
> + exec(@preargs, Samba::bindir_path($self, "nmbd"), "-F", "--no-process-group", "--log-stdout", "-s", $env_vars->{SERVERCONFFILE},
> + "-l", $env_vars->{LOGDIR}, @optargs) or die("Unable to start nmbd: $!");
> }
> $env_vars->{NMBD_TL_PID} = $pid;
> write_pid($env_vars, "nmbd", $pid);
> @@ -707,7 +708,8 @@ sub check_or_start($$$$$) {
> close($env_vars->{STDIN_PIPE});
> open STDIN, ">&", \*STDIN_READER or die "can't dup STDIN_READER to STDIN: $!";
>
> - exec(@preargs, Samba::bindir_path($self, "winbindd"), "-F", "--no-process-group", "--stdout", "-s", $env_vars->{SERVERCONFFILE}, @optargs) or die("Unable to start winbindd: $!");
> + exec(@preargs, Samba::bindir_path($self, "winbindd"), "-F", "--no-process-group", "--stdout", "-s", $env_vars->{SERVERCONFFILE},
> + "-l", $env_vars->{LOGDIR}, @optargs) or die("Unable to start winbindd: $!");
> }
> $env_vars->{WINBINDD_TL_PID} = $pid;
> write_pid($env_vars, "winbindd", $pid);
> @@ -757,7 +759,8 @@ sub check_or_start($$$$$) {
> close($env_vars->{STDIN_PIPE});
> open STDIN, ">&", \*STDIN_READER or die "can't dup STDIN_READER to STDIN: $!";
>
> - exec(@preargs, Samba::bindir_path($self, "smbd"), "-F", "--no-process-group", "--log-stdout", "-s", $env_vars->{SERVERCONFFILE}, @optargs) or die("Unable to start smbd: $!");
> + exec(@preargs, Samba::bindir_path($self, "smbd"), "-F", "--no-process-group", "--log-stdout", "-s", $env_vars->{SERVERCONFFILE},
> + "-l", $env_vars->{LOGDIR}, @optargs) or die("Unable to start smbd: $!");
> }
> $env_vars->{SMBD_TL_PID} = $pid;
> write_pid($env_vars, "smbd", $pid);
> @@ -1194,6 +1197,7 @@ domadmins:X:$gid_domadmins:
> $ret{NSS_WRAPPER_GROUP} = $nss_wrapper_group;
> $ret{NSS_WRAPPER_WINBIND_SO_PATH} = Samba::nss_wrapper_winbind_so_path($self);
> $ret{LOCAL_PATH} = "$shrdir";
> + $ret{LOGDIR} = $logdir;
>
> return \%ret;
> }
> --
> 1.7.9.5
>

> >From 031160dc983e92925b344961eccd596672727859 Mon Sep 17 00:00:00 2001
> From: Gregor Beck <gb...@sernet.de>
> Date: Mon, 22 Apr 2013 13:33:00 +0200
> Subject: [PATCH 2/2] s3: let the effective user of the parent smbd own the
> core dump dir.
>
> This prevents the chown to fail in testenv and seems to do the right thing in
> normal operation.
> ---
> source3/lib/dumpcore.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/source3/lib/dumpcore.c b/source3/lib/dumpcore.c
> index 90acc16..d41f258 100644
> --- a/source3/lib/dumpcore.c
> +++ b/source3/lib/dumpcore.c
> @@ -66,7 +66,7 @@ static char *get_default_corepath(const char *logbase, const char *progname)
> if (mkdir(tmp_corepath, 0700) == -1 && errno != EEXIST)
> goto err_out;
>
> - if (chown(tmp_corepath, getuid(), getgid()) == -1)
> + if (chown(tmp_corepath, geteuid(), getegid()) == -1)
> goto err_out;
>
> if (chmod(tmp_corepath, 0700) == -1)
> --
> 1.7.9.5
>

[Andrew Bartlett]

I'm sorry, but this looks wrong to me. If the mkdir didn't create the
directory with the correct owner, then how is a chown going to help?

The mkdir would have created the file with the current effective
uid/gid, so why change it again?

I know uid_wrappper is part of the game here, making these functions
return results that don't 'work', but if the original intent was to
ensure the files were changed from EUID (perhaps the user) to UID
(perhaps root?), then this seems quite wrong.

Perhaps it should just check the ownership against sec_initial_uid if
that isn't going to cause dependency pain?

Thanks,

Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org