Date: Mon, 16 Jan 2017 20:00:09 -0800 From: Gleb Smirnoff <glebius@FreeBSD.org> To: ngie@FreeBSD.org, Thor Steingrimsson <thor.steingrimsson@isilon.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r312331 - head/contrib/bsnmp/snmpd Message-ID: <20170117040009.GT2611@FreeBSD.org> In-Reply-To: <201701170352.v0H3qvNJ025187@repo.freebsd.org> References: <201701170352.v0H3qvNJ025187@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi, Ngie!
Please notice this regression when doing MFCs. Notice, that I'm
fixing this problem for a second time. See r240734. Please add
this to a regression suite.
Also, there is another regression in r310586, which actually isn't
your fault. Before r310586 in snmpd_input() there was stack variable
struct msghdr msg, which wasn't properly initialized. However, before
r310586 stack usage of descendants of snmpd_input() have hidden the
problem. After r310586, sendmsg() would fail since msg will have
garbage in msg.msg_control and msg.msg_controllen. Basicly, between
r310586 and r310655 doesn't work at all. And with r310655 it is
working again on 0.0.0.0 and doesn't work on any specific address.
So, if you plan to do MFC r310586 separately from further revisions,
you need to add 2 lines to snmpd_input() with this MFC:
Index: main.c
===================================================================
--- main.c (revision 310586)
+++ main.c (working copy)
@@ -1192,6 +1192,8 @@ snmpd_input(struct port_input *pi, struct tport *t
msg.msg_iov = iov;
msg.msg_iovlen = 1;
msg.msg_flags = 0;
+ msg.msg_control = NULL;
+ msg.msg_controllen = 0;
iov[0].iov_base = sndbuf;
iov[0].iov_len = sndlen;
On Tue, Jan 17, 2017 at 03:52:57AM +0000, Gleb Smirnoff wrote:
T> Author: glebius
T> Date: Tue Jan 17 03:52:57 2017
T> New Revision: 312331
T> URL: https://svnweb.freebsd.org/changeset/base/312331
T>
T> Log:
T> Fix regression from r310655, which broke operation of bsnmpd if it is bound
T> to a non-wildcard address. As documented in ip(4), doing sendmsg(2) with
T> IP_SENDSRCADDR on a socket that is bound to non-wildcard address is
T> completely different to using this control message on a wildcard one.
T>
T> A fix is to add a bool to mark whether we did setsockopt(IP_RECVDSTADDR)
T> on the socket, and use IP_SENDSRCADDR control message only if we did.
T>
T> While here, garbage collect absolutely useless udp_recv() function that
T> establishes some structures on stack to never use them later.
T>
T> Modified:
T> head/contrib/bsnmp/snmpd/trans_udp.c
T> head/contrib/bsnmp/snmpd/trans_udp.h
T>
T> Modified: head/contrib/bsnmp/snmpd/trans_udp.c
T> ==============================================================================
T> --- head/contrib/bsnmp/snmpd/trans_udp.c Tue Jan 17 03:44:45 2017 (r312330)
T> +++ head/contrib/bsnmp/snmpd/trans_udp.c Tue Jan 17 03:52:57 2017 (r312331)
T> @@ -34,6 +34,7 @@
T> #include <sys/queue.h>
T> #include <sys/ucred.h>
T>
T> +#include <stdbool.h>
T> #include <stdlib.h>
T> #include <syslog.h>
T> #include <string.h>
T> @@ -119,13 +120,15 @@ udp_init_port(struct tport *tp)
T> addr.sin_port = htons(p->port);
T> addr.sin_family = AF_INET;
T> addr.sin_len = sizeof(addr);
T> - if (addr.sin_addr.s_addr == INADDR_ANY &&
T> - setsockopt(p->input.fd, IPPROTO_IP, IP_RECVDSTADDR, &on,
T> - sizeof(on)) == -1) {
T> - syslog(LOG_ERR, "setsockopt(IP_RECVDSTADDR): %m");
T> - close(p->input.fd);
T> - p->input.fd = -1;
T> - return (SNMP_ERR_GENERR);
T> + if (addr.sin_addr.s_addr == INADDR_ANY) {
T> + if (setsockopt(p->input.fd, IPPROTO_IP, IP_RECVDSTADDR, &on,
T> + sizeof(on)) == -1) {
T> + syslog(LOG_ERR, "setsockopt(IP_RECVDSTADDR): %m");
T> + close(p->input.fd);
T> + p->input.fd = -1;
T> + return (SNMP_ERR_GENERR);
T> + }
T> + p->recvdstaddr = true;
T> }
T> if (bind(p->input.fd, (struct sockaddr *)&addr, sizeof(addr))) {
T> if (errno == EADDRNOTAVAIL) {
T> @@ -218,7 +221,6 @@ udp_send(struct tport *tp, const u_char
T> {
T> struct udp_port *p = (struct udp_port *)tp;
T> struct cmsghdr *cmsg;
T> - struct in_addr *src_addr;
T> struct msghdr msg;
T> char cbuf[CMSG_SPACE(sizeof(struct in_addr))];
T> struct iovec iov;
T> @@ -231,15 +233,20 @@ udp_send(struct tport *tp, const u_char
T> msg.msg_iovlen = 1;
T> msg.msg_name = __DECONST(void *, addr);
T> msg.msg_namelen = addrlen;
T> - msg.msg_control = cbuf;
T> - msg.msg_controllen = sizeof(cbuf);
T>
T> - cmsg = CMSG_FIRSTHDR(&msg);
T> - cmsg->cmsg_level = IPPROTO_IP;
T> - cmsg->cmsg_type = IP_SENDSRCADDR;
T> - cmsg->cmsg_len = CMSG_LEN(sizeof(struct in_addr));
T> - src_addr = (struct in_addr *)(void*)CMSG_DATA(cmsg);
T> - memcpy(src_addr, &p->recv_addr, sizeof(struct in_addr));
T> + if (p->recvdstaddr) {
T> + msg.msg_control = cbuf;
T> + msg.msg_controllen = sizeof(cbuf);
T> +
T> + cmsg = CMSG_FIRSTHDR(&msg);
T> + cmsg->cmsg_level = IPPROTO_IP;
T> + cmsg->cmsg_type = IP_SENDSRCADDR;
T> + cmsg->cmsg_len = CMSG_LEN(sizeof(struct in_addr));
T> + memcpy(CMSG_DATA(cmsg), &p->dstaddr, sizeof(struct in_addr));
T> + } else {
T> + msg.msg_control = NULL;
T> + msg.msg_controllen = 0;
T> + }
T>
T> return (sendmsg(p->input.fd, &msg, 0));
T> }
T> @@ -260,11 +267,12 @@ check_priv_dgram(struct port_input *pi,
T> * Each receive should return one datagram.
T> */
T> static ssize_t
T> -recv_dgram(struct port_input *pi, struct in_addr *laddr)
T> +udp_recv(struct tport *tp, struct port_input *pi)
T> {
T> u_char embuf[1000];
T> char cbuf[CMSG_SPACE(SOCKCREDSIZE(CMGROUP_MAX)) +
T> CMSG_SPACE(sizeof(struct in_addr))];
T> + struct udp_port *p = (struct udp_port *)tp;
T> struct msghdr msg;
T> struct iovec iov[1];
T> ssize_t len;
T> @@ -316,7 +324,8 @@ recv_dgram(struct port_input *pi, struct
T> cmsg = CMSG_NXTHDR(&msg, cmsg)) {
T> if (cmsg->cmsg_level == IPPROTO_IP &&
T> cmsg->cmsg_type == IP_RECVDSTADDR)
T> - memcpy(laddr, CMSG_DATA(cmsg), sizeof(struct in_addr));
T> + memcpy(&p->dstaddr, CMSG_DATA(cmsg),
T> + sizeof(struct in_addr));
T> if (cmsg->cmsg_level == SOL_SOCKET &&
T> cmsg->cmsg_type == SCM_CREDS)
T> cred = (struct sockcred *)CMSG_DATA(cmsg);
T> @@ -329,42 +338,6 @@ recv_dgram(struct port_input *pi, struct
T> }
T>
T> /*
T> - * Receive something
T> - */
T> -static ssize_t
T> -udp_recv(struct tport *tp, struct port_input *pi)
T> -{
T> - struct udp_port *p = (struct udp_port *)tp;
T> - struct cmsghdr *cmsgp;
T> - struct in_addr *laddr;
T> - struct msghdr msg;
T> - char cbuf[CMSG_SPACE(sizeof(struct in_addr))];
T> - ssize_t ret;
T> -
T> - memset(cbuf, 0, sizeof(cbuf));
T> -
T> - msg.msg_control = cbuf;
T> - msg.msg_controllen = sizeof(cbuf);
T> -
T> - cmsgp = CMSG_FIRSTHDR(&msg);
T> - cmsgp->cmsg_len = CMSG_LEN(sizeof(struct in_addr));
T> - cmsgp->cmsg_level = IPPROTO_IP;
T> - cmsgp->cmsg_type = IP_SENDSRCADDR;
T> - laddr = (struct in_addr *)CMSG_DATA(cmsgp);
T> -
T> - ret = recv_dgram(pi, laddr);
T> -
T> - memcpy(&p->recv_addr, laddr, sizeof(struct in_addr));
T> -
T> - if (laddr->s_addr == INADDR_ANY) {
T> - msg.msg_control = NULL;
T> - msg.msg_controllen = 0;
T> - }
T> -
T> - return (ret);
T> -}
T> -
T> -/*
T> * Port table
T> */
T> int
T>
T> Modified: head/contrib/bsnmp/snmpd/trans_udp.h
T> ==============================================================================
T> --- head/contrib/bsnmp/snmpd/trans_udp.h Tue Jan 17 03:44:45 2017 (r312330)
T> +++ head/contrib/bsnmp/snmpd/trans_udp.h Tue Jan 17 03:52:57 2017 (r312331)
T> @@ -39,7 +39,9 @@ struct udp_port {
T> struct port_input input; /* common input stuff */
T>
T> struct sockaddr_in ret; /* the return address */
T> - struct in_addr recv_addr; /* the address the request was sent to */
T> +
T> + bool recvdstaddr; /* IP_RECVDSTADDR is on */
T> + struct in_addr dstaddr; /* address the request was sent to */
T> };
T>
T> /* argument for open call */
T> _______________________________________________
T> svn-src-all@freebsd.org mailing list
T> https://lists.freebsd.org/mailman/listinfo/svn-src-all
T> To unsubscribe, send any mail to "svn-src-all-unsubscribe@freebsd.org"
--
Totus tuus, Glebius.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170117040009.GT2611>