On Sat, Jan 16, 2016 at 12:19:18PM +0100, Landry Breuil wrote:
> On Sat, Jan 16, 2016 at 11:47:33AM +0100, Landry Breuil wrote:
> > On Sat, Jan 16, 2016 at 11:40:35AM +0100, Landry Breuil wrote:
> > > Hi,
> > >
> > > playing with ldapd, i noticed that upon exit, the child process (ldape)
> > > is aborted by pledge:
> > >
> > > ldapd(10229): syscall 10 "cpath"
> > >
> > > and /var/run/ldapi / ldapd.sock are left behind.
> > >
> > > ktracing and looking at the code, it seems control_cleanup in
> > > control.c:117 tries to unlink the control socket, and i havent found yet
> > > the code that tries to remove the listening ldapi socket (i think
> > > there's none).
> > >
> > > Interestingly, adding cpath to the pledge() call in ldape.c makes the
> > > abort go away, but the two sockets are still left behind.. so i'm not
> > > sure this is the right way to go, and there's probably more to do.
> >
> > Thinking more about it (after more coffee), the child process can't
> > unlink those socks since it has chroot'ed.. so the cleanup has to be
> > done by the parent process.
>
> And here's a diff that should do the right thing, tested with two unix
> sockets in ldapd.conf and an alternative control socket via -s, all
> removed upon exit.
I think I ran into this while adding pledge() to ldapd, but forgot to do
anything about it. One question below, otherwise ok by me.
>
> Landry
> ? ktrace.out
> Index: control.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ldapd/control.c,v
> retrieving revision 1.12
> diff -u -r1.12 control.c
> --- control.c 24 Dec 2015 17:47:57 -0000 1.12
> +++ control.c 16 Jan 2016 11:17:42 -0000
> @@ -114,7 +114,6 @@
> return;
> event_del(&cs->cs_ev);
> event_del(&cs->cs_evt);
> - (void)unlink(cs->cs_name);
> }
>
> /* ARGSUSED */
> Index: ldapd.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ldapd/ldapd.c,v
> retrieving revision 1.15
> diff -u -r1.15 ldapd.c
> --- ldapd.c 24 Dec 2015 17:47:57 -0000 1.15
> +++ ldapd.c 16 Jan 2016 11:17:42 -0000
> @@ -17,6 +17,7 @@
> */
>
> #include <sys/queue.h>
> +#include <sys/un.h>
> #include <sys/types.h>
> #include <sys/wait.h>
>
> @@ -45,6 +46,7 @@
> static void ldapd_auth_request(struct imsgev *iev, struct imsg *imsg);
> static void ldapd_open_request(struct imsgev *iev, struct imsg *imsg);
> static void ldapd_log_verbose(struct imsg *imsg);
> +static void ldapd_cleanup(char *);
>
> struct ldapd_stats stats;
> pid_t ldape_pid;
> @@ -213,9 +215,31 @@
> err(1, "pledge");
>
> event_dispatch();
> +
> + ldapd_cleanup(csockpath);
> log_debug("ldapd: exiting");
>
> return 0;
> +}
> +
> +static void
> +ldapd_cleanup(char * csockpath)
> +{
> + struct listener *l;
> + struct sockaddr_un *sun = NULL;
> +
> + /* Remove control socket. */
> + (void)unlink(csockpath);
> +
> + /* Remove unix listening sockets. */
> + TAILQ_FOREACH(l, &conf->listeners, entry) {
> + if (l->ss.ss_family == AF_UNIX) {
> + sun = (struct sockaddr_un *)&l->ss;
> + log_info("ldapd: removing unix socket %s",
> sun->sun_path);
> + if (unlink(sun->sun_path) == -1 && errno != ENOENT)
> + fatal("ldapd: unlink");
The only thing I'm not really sure about is calling fatal() in the
middle of cleanup just before exiting - wouldn't it be better to
keep going?
> + }
> + }
> }
>
> static void
