From owner-svn-src-all@freebsd.org Tue Jan 17 04:00:16 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id E6AB9CB329A; Tue, 17 Jan 2017 04:00:16 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (glebi.us [96.95.210.25]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "cell.glebi.us", Issuer "cell.glebi.us" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id CB921179A; Tue, 17 Jan 2017 04:00:16 +0000 (UTC) (envelope-from glebius@FreeBSD.org) Received: from cell.glebi.us (localhost [127.0.0.1]) by cell.glebi.us (8.15.2/8.15.2) with ESMTPS id v0H409mH044827 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Mon, 16 Jan 2017 20:00:09 -0800 (PST) (envelope-from glebius@FreeBSD.org) Received: (from glebius@localhost) by cell.glebi.us (8.15.2/8.15.2/Submit) id v0H409XA044826; Mon, 16 Jan 2017 20:00:09 -0800 (PST) (envelope-from glebius@FreeBSD.org) X-Authentication-Warning: cell.glebi.us: glebius set sender to glebius@FreeBSD.org using -f Date: Mon, 16 Jan 2017 20:00:09 -0800 From: Gleb Smirnoff To: ngie@FreeBSD.org, Thor Steingrimsson 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> References: <201701170352.v0H3qvNJ025187@repo.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201701170352.v0H3qvNJ025187@repo.freebsd.org> User-Agent: Mutt/1.7.2 (2016-11-26) X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 Jan 2017 04:00:17 -0000 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 T> #include T> T> +#include T> #include T> #include T> #include 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.