mdns stuff in intercept_finish is broken

[Mark Atwood]

I agree.  When you know you are heading for a prompt exit from a signal,
don't try to clean up anything, especially the alloc arena.  Log a message
fast, and then exit.

It is my own painful experience that when terminating signal is received,
especially one generated by the OS or the kernel, one often can no longer
trust the coherency of alloc, of stdio, of the in-process DNS/name cache.
Thus, don't try to clean anything up, don't try to look anything up, and
hope that you can get one more write to stderr and/or syslog, and just exit.

Also, there are few things as rage inducing to a sysadmin or to a devops
developer as a process that thinks it knows better than you do when it
receives a signal, or that now is a good time to spend time doing anything
other than exiting.

The sysadmin or devops developer will forgive all the alloc leak warnings.

POSIX guarantees that process memory is reclaimed, and that all fd's get
closed.  Use that guarantee.

If the day comes that we run on an OS that fails that guarantee, instead of
trying to work around it, instead we will file bugs against that OS, and
not mess up NTPsec to cover for their mistake.

..m

On Mon, Nov 30, 2015 at 5:39 AM Eric S. Raymond <esr at thyrsus.com> wrote:

> Hal Murray <hmurray at megapathdsl.net>:
> >
> > I think it only gets tested on NetBSD.
> >
> > The first problem is that it doesn't include the header file.
> > The second is that mdns is over in ntpd.c
> >
> > I think the idea of moving that much actual code over there is a bad
> idea.
>
> I reverted that change last night.  You're right, it was a bad idea.
>
> > This looks fishy :
> >         sig_desc = NULL;
> >         sig_desc = strsignal(sig);
> >         if (sig_desc == NULL)
> >                 sig_desc = "";
> >
> > It's leftover from when strsignal might not exist and didn't get cleaned
> up
> > when you removed that ifdef test.  My man page says some systems (not
> Linux)
> > might return NULL on an invalid signal.  I assume anything passed to that
> > routine will be valid.
>
> Yes, I guess the "sig_desc = NULL;" can go
>
> > There are 2 ways into that code.  One is via a kill command, probably
> from
> > something like "service ntpd stop".  The other is from a SIGBUS.  The
> SIGBUS
> > case should probably bypass the cleanup and exit non-zero.
> >
> > I think the idea of doing more than a simple log message and exit is to
> free
> > up all the mallocs so the tool that monitors malloc usage would see a
> close
> > to clean result at exit and everything left would be lost memory.  I
> don't
> > see the code I expect.  There is a lot of mallocing in the config file
> parser
> > and big chunks of memory for the mrulist.  The config stuff is using an
> > atexit-hook.  That's probably a bad idea if we are going to catch SIGBUS
> and
> > expect to do anything.
>
> You're almost certainly right about all this.  Feel free to clean up
> as you like; I can't get very excited about it, as I judge the
> objective (free up all mallocs before exit) to be unachievable in
> practice given NTP's level of allocation complexity and multiple exit
> paths.
> --
>                 <a href="http://www.catb.org/~esr/">Eric S. Raymond</a>
> _______________________________________________
> devel mailing list
> devel at ntpsec.org
> http://lists.ntpsec.org/mailman/listinfo/devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ntpsec.org/pipermail/devel/attachments/20151201/76e3113e/attachment.html>