Correcty reloading unresolved host in syslogd @Conf lines

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

Correcty reloading unresolved host in syslogd @Conf lines

sven falempin
This was looked at before.
Did not get through.

--- /usr/src/usr.sbin/syslogd/syslogd.c Wed May 13 19:17:17 2020
+++ ./syslogd.c Mon Feb 10 16:05:59 2020
@@ -2416,6 +2416,7 @@
        s = 0;
        strlcpy(progblock, "*", sizeof(progblock));
        strlcpy(hostblock, "*", sizeof(hostblock));
+       send_udp = send_udp6 = 0;
        while (getline(&cline, &s, cf) != -1) {
                /*
                 * check for end-of-section, comments, strip off trailing
@@ -2755,6 +2756,9 @@
                    sizeof(f->f_un.f_forw.f_addr)) != 0) {
                        log_warnx("bad hostname \"%s\"",
                            f->f_un.f_forw.f_loghost);
+                       /* DNS lookup may work after SIGHUP, keep sockets */
+                       if (strncmp(proto, "udp", 3) == 0)
+                               send_udp = send_udp6 = 1;
                        break;
                }
                f->f_file = -1;

It helps with @ domain in conf.

As explain a Failed DNS request at boot can work later.

--
--
---------------------------------------------------------------------------------------------------------------------
Knowing is not enough; we must apply. Willing is not enough; we must do
Reply | Threaded
Open this post in threaded view
|

Re: Correcty reloading unresolved host in syslogd @Conf lines

Alexander Bluhm
On Sat, May 16, 2020 at 07:23:37PM -0400, sven falempin wrote:
> This was looked at before.
> Did not get through.

The posted diff was not my final solution.  But yes, the issue was
forgotten.  So I would suggest this.

When DNS lookup of an UDP loghost failed, syslogd(8) did close the
UDP sockets for sending messages.  Keep the sockets open in this
case.  Then they can be used if DNS is working during the next
SIGHUP.

ok?

bluhm

Index: usr.sbin/syslogd/syslogd.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.262
diff -u -p -r1.262 syslogd.c
--- usr.sbin/syslogd/syslogd.c 5 Jul 2019 13:23:27 -0000 1.262
+++ usr.sbin/syslogd/syslogd.c 9 Feb 2020 20:25:20 -0000
@@ -853,20 +853,6 @@ main(int argc, char *argv[])
  event_add(ev_udp, NULL);
  if (fd_udp6 != -1)
  event_add(ev_udp6, NULL);
- } else {
- /*
- * If generic UDP file descriptors are used neither
- * for receiving nor for sending, close them.  Then
- * there is no useless *.514 in netstat.
- */
- if (fd_udp != -1 && !send_udp) {
- close(fd_udp);
- fd_udp = -1;
- }
- if (fd_udp6 != -1 && !send_udp6) {
- close(fd_udp6);
- fd_udp6 = -1;
- }
  }
  for (i = 0; i < nbind; i++)
  if (fd_bind[i] != -1)
@@ -2416,6 +2402,7 @@ init(void)
  s = 0;
  strlcpy(progblock, "*", sizeof(progblock));
  strlcpy(hostblock, "*", sizeof(hostblock));
+ send_udp = send_udp6 = 0;
  while (getline(&cline, &s, cf) != -1) {
  /*
  * check for end-of-section, comments, strip off trailing
@@ -2508,6 +2495,22 @@ init(void)
  Initialized = 1;
  dropped_warn(&init_dropped, "during initialization");

+ if (SecureMode) {
+ /*
+ * If generic UDP file descriptors are used neither
+ * for receiving nor for sending, close them.  Then
+ * there is no useless *.514 in netstat.
+ */
+ if (fd_udp != -1 && !send_udp) {
+ close(fd_udp);
+ fd_udp = -1;
+ }
+ if (fd_udp6 != -1 && !send_udp6) {
+ close(fd_udp6);
+ fd_udp6 = -1;
+ }
+ }
+
  if (Debug) {
  SIMPLEQ_FOREACH(f, &Files, f_next) {
  for (i = 0; i <= LOG_NFACILITIES; i++)
@@ -2755,6 +2758,13 @@ cfline(char *line, char *progblock, char
     sizeof(f->f_un.f_forw.f_addr)) != 0) {
  log_warnx("bad hostname \"%s\"",
     f->f_un.f_forw.f_loghost);
+ /* DNS lookup may work after SIGHUP, keep sockets */
+ if (strcmp(proto, "udp") == 0)
+ send_udp = send_udp6 = 1;
+ else if (strcmp(proto, "udp4") == 0)
+ send_udp = 1;
+ else if (strcmp(proto, "udp6") == 0)
+ send_udp6 = 1;
  break;
  }
  f->f_file = -1;

Reply | Threaded
Open this post in threaded view
|

Re: Correcty reloading unresolved host in syslogd @Conf lines

sven falempin
On Mon, May 18, 2020 at 5:31 AM Alexander Bluhm <[hidden email]>
wrote:

> On Sat, May 16, 2020 at 07:23:37PM -0400, sven falempin wrote:
> > This was looked at before.
> > Did not get through.
>
> The posted diff was not my final solution.  But yes, the issue was
> forgotten.  So I would suggest this.
>
> When DNS lookup of an UDP loghost failed, syslogd(8) did close the
> UDP sockets for sending messages.  Keep the sockets open in this
> case.  Then they can be used if DNS is working during the next
> SIGHUP.
>
> ok?
>
> bluhm
>
> Index: usr.sbin/syslogd/syslogd.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.262
> diff -u -p -r1.262 syslogd.c
> --- usr.sbin/syslogd/syslogd.c  5 Jul 2019 13:23:27 -0000       1.262
> +++ usr.sbin/syslogd/syslogd.c  9 Feb 2020 20:25:20 -0000
> @@ -853,20 +853,6 @@ main(int argc, char *argv[])
>                         event_add(ev_udp, NULL);
>                 if (fd_udp6 != -1)
>                         event_add(ev_udp6, NULL);
> -       } else {
> -               /*
> -                * If generic UDP file descriptors are used neither
> -                * for receiving nor for sending, close them.  Then
> -                * there is no useless *.514 in netstat.
> -                */
> -               if (fd_udp != -1 && !send_udp) {
> -                       close(fd_udp);
> -                       fd_udp = -1;
> -               }
> -               if (fd_udp6 != -1 && !send_udp6) {
> -                       close(fd_udp6);
> -                       fd_udp6 = -1;
> -               }
>         }
>         for (i = 0; i < nbind; i++)
>                 if (fd_bind[i] != -1)
> @@ -2416,6 +2402,7 @@ init(void)
>         s = 0;
>         strlcpy(progblock, "*", sizeof(progblock));
>         strlcpy(hostblock, "*", sizeof(hostblock));
> +       send_udp = send_udp6 = 0;
>         while (getline(&cline, &s, cf) != -1) {
>                 /*
>                  * check for end-of-section, comments, strip off trailing
> @@ -2508,6 +2495,22 @@ init(void)
>         Initialized = 1;
>         dropped_warn(&init_dropped, "during initialization");
>
> +       if (SecureMode) {
> +               /*
> +                * If generic UDP file descriptors are used neither
> +                * for receiving nor for sending, close them.  Then
> +                * there is no useless *.514 in netstat.
> +                */
> +               if (fd_udp != -1 && !send_udp) {
> +                       close(fd_udp);
> +                       fd_udp = -1;
> +               }
> +               if (fd_udp6 != -1 && !send_udp6) {
> +                       close(fd_udp6);
> +                       fd_udp6 = -1;
> +               }
> +       }
> +
>         if (Debug) {
>                 SIMPLEQ_FOREACH(f, &Files, f_next) {
>                         for (i = 0; i <= LOG_NFACILITIES; i++)
> @@ -2755,6 +2758,13 @@ cfline(char *line, char *progblock, char
>                     sizeof(f->f_un.f_forw.f_addr)) != 0) {
>                         log_warnx("bad hostname \"%s\"",
>                             f->f_un.f_forw.f_loghost);
> +                       /* DNS lookup may work after SIGHUP, keep sockets
> */
> +                       if (strcmp(proto, "udp") == 0)
> +                               send_udp = send_udp6 = 1;
> +                       else if (strcmp(proto, "udp4") == 0)
> +                               send_udp = 1;
> +                       else if (strcmp(proto, "udp6") == 0)
> +                               send_udp6 = 1;
>                         break;
>                 }
>                 f->f_file = -1;
>

@OK


? Will it goes into base this time ?

tl;dr

_Patch_

--------------------------
|Index: usr.sbin/syslogd/syslogd.c
|===================================================================
|RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
|retrieving revision 1.262
|diff -u -p -r1.262 syslogd.c
|--- usr.sbin/syslogd/syslogd.c  5 Jul 2019 13:23:27 -0000       1.262
|+++ usr.sbin/syslogd/syslogd.c  9 Feb 2020 20:25:20 -0000
--------------------------
Patching file usr.sbin/syslogd/syslogd.c using Plan A...
Hunk #1 succeeded at 853.
Hunk #2 succeeded at 2402.
Hunk #3 succeeded at 2495.
Hunk #4 succeeded at 2758.

_Install run debug with @totalcrappy _

logline: pri 053, flags 0x4, from current, msg syslogd[33975]: bad hostname
"@totalcrappy"
[..]
Logging to UNUSED

Kill _ put totalcrappy in etc/hosts to simluate dns fix _

X X X X X X X X X X X X X X X X 8 X X X X X X X X FORWUDP: @totalcrappy
logline: pri 056, flags 0x4, from current, msg syslogd[33975]: restart

Logging to FORWUDP @totalcrappy

OK

( current version does :
Logging to FORWUDP @totalcrappy
logline: pri 053, flags 0x4, from current, msg syslogd[18624]: sendto
"@totalcrappy": Bad file descriptor
)

--
--
---------------------------------------------------------------------------------------------------------------------
Knowing is not enough; we must apply. Willing is not enough; we must do
Reply | Threaded
Open this post in threaded view
|

Re: Correcty reloading unresolved host in syslogd @Conf lines

Alexander Bluhm
On Wed, May 20, 2020 at 09:29:54PM -0400, sven falempin wrote:
> ? Will it goes into base this time ?

I need an OK from a developer.  Anyone?

bluhm

> On Mon, May 18, 2020 at 5:31 AM Alexander Bluhm <[hidden email]>
> > Index: usr.sbin/syslogd/syslogd.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
> > retrieving revision 1.262
> > diff -u -p -r1.262 syslogd.c
> > --- usr.sbin/syslogd/syslogd.c  5 Jul 2019 13:23:27 -0000       1.262
> > +++ usr.sbin/syslogd/syslogd.c  9 Feb 2020 20:25:20 -0000
> > @@ -853,20 +853,6 @@ main(int argc, char *argv[])
> >                         event_add(ev_udp, NULL);
> >                 if (fd_udp6 != -1)
> >                         event_add(ev_udp6, NULL);
> > -       } else {
> > -               /*
> > -                * If generic UDP file descriptors are used neither
> > -                * for receiving nor for sending, close them.  Then
> > -                * there is no useless *.514 in netstat.
> > -                */
> > -               if (fd_udp != -1 && !send_udp) {
> > -                       close(fd_udp);
> > -                       fd_udp = -1;
> > -               }
> > -               if (fd_udp6 != -1 && !send_udp6) {
> > -                       close(fd_udp6);
> > -                       fd_udp6 = -1;
> > -               }
> >         }
> >         for (i = 0; i < nbind; i++)
> >                 if (fd_bind[i] != -1)
> > @@ -2416,6 +2402,7 @@ init(void)
> >         s = 0;
> >         strlcpy(progblock, "*", sizeof(progblock));
> >         strlcpy(hostblock, "*", sizeof(hostblock));
> > +       send_udp = send_udp6 = 0;
> >         while (getline(&cline, &s, cf) != -1) {
> >                 /*
> >                  * check for end-of-section, comments, strip off trailing
> > @@ -2508,6 +2495,22 @@ init(void)
> >         Initialized = 1;
> >         dropped_warn(&init_dropped, "during initialization");
> >
> > +       if (SecureMode) {
> > +               /*
> > +                * If generic UDP file descriptors are used neither
> > +                * for receiving nor for sending, close them.  Then
> > +                * there is no useless *.514 in netstat.
> > +                */
> > +               if (fd_udp != -1 && !send_udp) {
> > +                       close(fd_udp);
> > +                       fd_udp = -1;
> > +               }
> > +               if (fd_udp6 != -1 && !send_udp6) {
> > +                       close(fd_udp6);
> > +                       fd_udp6 = -1;
> > +               }
> > +       }
> > +
> >         if (Debug) {
> >                 SIMPLEQ_FOREACH(f, &Files, f_next) {
> >                         for (i = 0; i <= LOG_NFACILITIES; i++)
> > @@ -2755,6 +2758,13 @@ cfline(char *line, char *progblock, char
> >                     sizeof(f->f_un.f_forw.f_addr)) != 0) {
> >                         log_warnx("bad hostname \"%s\"",
> >                             f->f_un.f_forw.f_loghost);
> > +                       /* DNS lookup may work after SIGHUP, keep sockets
> > */
> > +                       if (strcmp(proto, "udp") == 0)
> > +                               send_udp = send_udp6 = 1;
> > +                       else if (strcmp(proto, "udp4") == 0)
> > +                               send_udp = 1;
> > +                       else if (strcmp(proto, "udp6") == 0)
> > +                               send_udp6 = 1;
> >                         break;
> >                 }
> >                 f->f_file = -1;
> >

Reply | Threaded
Open this post in threaded view
|

Re: Correcty reloading unresolved host in syslogd @Conf lines

Todd C. Miller-3
On Fri, 22 May 2020 12:08:28 +0200, Alexander Bluhm wrote:

> On Wed, May 20, 2020 at 09:29:54PM -0400, sven falempin wrote:
> > ? Will it goes into base this time ?
>
> I need an OK from a developer.  Anyone?

I'm a little confused by the protocol handling in cfline.

        if (strcmp(proto, "udp") == 0) {
                if (fd_udp == -1)
                        proto = "udp6";
                if (fd_udp6 == -1)
                        proto = "udp4";
                ipproto = proto;
        }

Doesn't that mean that in the default case if a syslog server is
not reachable, proto will end up being set to "udp4" and not "udp"?
If so, then your diff will only retry udp4 on SIGHUP instead of
both udp4 and udp6.

 - todd

Reply | Threaded
Open this post in threaded view
|

Re: Correcty reloading unresolved host in syslogd @Conf lines

Alexander Bluhm
On Fri, May 22, 2020 at 07:38:30AM -0600, Todd C. Miller wrote:

> I'm a little confused by the protocol handling in cfline.
>
> if (strcmp(proto, "udp") == 0) {
> if (fd_udp == -1)
> proto = "udp6";
> if (fd_udp6 == -1)
> proto = "udp4";
> ipproto = proto;
> }
>
> Doesn't that mean that in the default case if a syslog server is
> not reachable, proto will end up being set to "udp4" and not "udp"?
> If so, then your diff will only retry udp4 on SIGHUP instead of
> both udp4 and udp6.

What do you mean by "not reachable"?  As we do not connect(2) and
ignore most errors of sendto(2), syslogd(8) knows nothing about
reachabiliy.  I guess you mean "if DNS lookup fails".

fd_udp and fd_udp6 should never become -1 as we cannot reopen them.
If fd_udp6 is -1 we have to restrict ourselves to "udp4".  But it
is better to move this code out of the big if else block.  Then we
get the "no udp4" warning if something went wrong.

There was another problem with my diff.  If DNS server switches
between A and AAAA answers after SIGHUP, the wrong socket has been
closed.  It is better to close the sockets based only on configuration,
not on runtime DNS.  Note that when the config file changes, syslogd
re-execs itself and we start with fresh sockets.

New diff, move the send_udp = 1 a bit up to the config logic.

ok?

bluhm

Index: usr.sbin/syslogd/syslogd.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.262
diff -u -p -r1.262 syslogd.c
--- usr.sbin/syslogd/syslogd.c 5 Jul 2019 13:23:27 -0000 1.262
+++ usr.sbin/syslogd/syslogd.c 22 May 2020 18:21:23 -0000
@@ -853,20 +853,6 @@ main(int argc, char *argv[])
  event_add(ev_udp, NULL);
  if (fd_udp6 != -1)
  event_add(ev_udp6, NULL);
- } else {
- /*
- * If generic UDP file descriptors are used neither
- * for receiving nor for sending, close them.  Then
- * there is no useless *.514 in netstat.
- */
- if (fd_udp != -1 && !send_udp) {
- close(fd_udp);
- fd_udp = -1;
- }
- if (fd_udp6 != -1 && !send_udp6) {
- close(fd_udp6);
- fd_udp6 = -1;
- }
  }
  for (i = 0; i < nbind; i++)
  if (fd_bind[i] != -1)
@@ -2416,6 +2402,7 @@ init(void)
  s = 0;
  strlcpy(progblock, "*", sizeof(progblock));
  strlcpy(hostblock, "*", sizeof(hostblock));
+ send_udp = send_udp6 = 0;
  while (getline(&cline, &s, cf) != -1) {
  /*
  * check for end-of-section, comments, strip off trailing
@@ -2508,6 +2495,22 @@ init(void)
  Initialized = 1;
  dropped_warn(&init_dropped, "during initialization");

+ if (SecureMode) {
+ /*
+ * If generic UDP file descriptors are used neither
+ * for receiving nor for sending, close them.  Then
+ * there is no useless *.514 in netstat.
+ */
+ if (fd_udp != -1 && !send_udp) {
+ close(fd_udp);
+ fd_udp = -1;
+ }
+ if (fd_udp6 != -1 && !send_udp6) {
+ close(fd_udp6);
+ fd_udp6 = -1;
+ }
+ }
+
  if (Debug) {
  SIMPLEQ_FOREACH(f, &Files, f_next) {
  for (i = 0; i <= LOG_NFACILITIES; i++)
@@ -2704,20 +2707,24 @@ cfline(char *line, char *progblock, char
  }
  if (proto == NULL)
  proto = "udp";
- ipproto = proto;
  if (strcmp(proto, "udp") == 0) {
  if (fd_udp == -1)
  proto = "udp6";
  if (fd_udp6 == -1)
  proto = "udp4";
- ipproto = proto;
+ }
+ ipproto = proto;
+ if (strcmp(proto, "udp") == 0) {
+ send_udp = send_udp6 = 1;
  } else if (strcmp(proto, "udp4") == 0) {
+ send_udp = 1;
  if (fd_udp == -1) {
  log_warnx("no udp4 \"%s\"",
     f->f_un.f_forw.f_loghost);
  break;
  }
  } else if (strcmp(proto, "udp6") == 0) {
+ send_udp6 = 1;
  if (fd_udp6 == -1) {
  log_warnx("no udp6 \"%s\"",
     f->f_un.f_forw.f_loghost);
@@ -2761,11 +2768,9 @@ cfline(char *line, char *progblock, char
  if (strncmp(proto, "udp", 3) == 0) {
  switch (f->f_un.f_forw.f_addr.ss_family) {
  case AF_INET:
- send_udp = 1;
  f->f_file = fd_udp;
  break;
  case AF_INET6:
- send_udp6 = 1;
  f->f_file = fd_udp6;
  break;
  }

Reply | Threaded
Open this post in threaded view
|

Re: Correcty reloading unresolved host in syslogd @Conf lines

Todd C. Miller-3
On Fri, 22 May 2020 21:29:43 +0200, Alexander Bluhm wrote:

> On Fri, May 22, 2020 at 07:38:30AM -0600, Todd C. Miller wrote:
> > I'm a little confused by the protocol handling in cfline.
> >
> > if (strcmp(proto, "udp") == 0) {
> > if (fd_udp == -1)
> > proto = "udp6";
> > if (fd_udp6 == -1)
> > proto = "udp4";
> > ipproto = proto;
> > }
> >
> > Doesn't that mean that in the default case if a syslog server is
> > not reachable, proto will end up being set to "udp4" and not "udp"?
> > If so, then your diff will only retry udp4 on SIGHUP instead of
> > both udp4 and udp6.
>
> What do you mean by "not reachable"?  As we do not connect(2) and
> ignore most errors of sendto(2), syslogd(8) knows nothing about
> reachabiliy.  I guess you mean "if DNS lookup fails".

Sorry, yes, I meant if the DNS lookup fails.

> fd_udp and fd_udp6 should never become -1 as we cannot reopen them.
> If fd_udp6 is -1 we have to restrict ourselves to "udp4".  But it
> is better to move this code out of the big if else block.  Then we
> get the "no udp4" warning if something went wrong.
>
> There was another problem with my diff.  If DNS server switches
> between A and AAAA answers after SIGHUP, the wrong socket has been
> closed.  It is better to close the sockets based only on configuration,
> not on runtime DNS.  Note that when the config file changes, syslogd
> re-execs itself and we start with fresh sockets.
>
> New diff, move the send_udp = 1 a bit up to the config logic.

OK millert@

 - todd