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;
}
