On Sat, Jan 16, 2016 at 11:15:30PM +1000, Jonathan Matthew wrote:
> 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?
Good point - the other option would be to log_warn() ? Or just call
unlink() without checking the return code like its' done for csockpath ?
Landry
