From c3375d0501455758c2e292efb0a64aa45c69e38d Mon Sep 17 00:00:00 2001 From: Andrew Bettison Date: Tue, 19 Jan 2016 04:05:46 +1030 Subject: [PATCH] Fix MDP client packet receive bug on OSX Was failing because recvmsg(2) on OSX does not nul terminate the pathname of local (AF_UNIX) sockets in the returned sockaddr buffer. Zerofilling the buffer before calling recvmsg() does the trick. In the process, improved debug and error logging, replacing recvwithttl() with recv_message() and recv_message_frag(). The mdp_net.c source file was retired. --- mdp_client.c | 12 +++----- mdp_net.c | 70 --------------------------------------------- overlay_interface.c | 12 ++------ overlay_mdp.c | 12 ++------ socket.c | 64 ++++++++++++++++++++++++++++++++++------- socket.h | 10 +++---- sourcefiles.mk | 5 ++-- 7 files changed, 69 insertions(+), 116 deletions(-) delete mode 100644 mdp_net.c diff --git a/mdp_client.c b/mdp_client.c index 097fda15..ad05d2c1 100644 --- a/mdp_client.c +++ b/mdp_client.c @@ -323,12 +323,8 @@ int overlay_mdp_recv(int mdp_sockfd, overlay_mdp_frame *mdp, mdp_port_t port, in ssize_t len; mdp->packetTypeAndFlags = 0; set_nonblock(mdp_sockfd); - len = recvwithttl(mdp_sockfd, (unsigned char *)mdp, sizeof(overlay_mdp_frame), ttl, &recvaddr); - if (len == -1) - WHYF_perror("recvwithttl(%d,%p,%zu,&%d,%p(%s))", - mdp_sockfd, mdp, sizeof(overlay_mdp_frame), *ttl, - &recvaddr, alloca_socket_address(&recvaddr) - ); + + len = recv_message(mdp_sockfd, &recvaddr, ttl, (unsigned char *)mdp, sizeof(overlay_mdp_frame)); set_block(mdp_sockfd); if (len <= 0) return -1; // no packet received @@ -336,7 +332,7 @@ int overlay_mdp_recv(int mdp_sockfd, overlay_mdp_frame *mdp, mdp_port_t port, in // If the received address overflowed the buffer, then it cannot have come from the server, whose // address must always fit within a struct sockaddr_un. if (recvaddr.addrlen > sizeof recvaddr.store) - return WHY("reply did not come from server: address overrun"); + return WHYF("reply did not come from server %s: address overrun", alloca_socket_address(&mdp_addr)); // Compare the address of the sender with the address of our server, to ensure they are the same. // If the comparison fails, then try using realpath(3) on the sender address and compare again. @@ -346,7 +342,7 @@ int overlay_mdp_recv(int mdp_sockfd, overlay_mdp_frame *mdp, mdp_port_t port, in || cmp_sockaddr(&recvaddr, &mdp_addr) != 0 ) ) - return WHYF("reply did not come from server: %s", alloca_socket_address(&recvaddr)); + return WHYF("reply did not come from server %s: %s", alloca_socket_address(&mdp_addr), alloca_socket_address(&recvaddr)); // silently drop incoming packets for the wrong port number if (port>0 && port != mdp->out.dst.port){ diff --git a/mdp_net.c b/mdp_net.c deleted file mode 100644 index e79da4a3..00000000 --- a/mdp_net.c +++ /dev/null @@ -1,70 +0,0 @@ -/* -Copyright (C) 2013 Serval Project Inc. - -This program is free software; you can redistribute it and/or -modify it under the terms of the GNU General Public License -as published by the Free Software Foundation; either version 2 -of the License, or (at your option) any later version. - -This program is distributed in the hope that it will be useful, -but WITHOUT ANY WARRANTY; without even the implied warranty of -MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -GNU General Public License for more details. - -You should have received a copy of the GNU General Public License -along with this program; if not, write to the Free Software -Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. -*/ - -#include "socket.h" -#include "conf.h" -#include "log.h" -#include "debug.h" - -ssize_t recvwithttl(int sock,unsigned char *buffer, size_t bufferlen,int *ttl, struct socket_address *recvaddr) -{ - struct msghdr msg; - struct iovec iov[1]; - struct cmsghdr cmsgcmsg[16]; - iov[0].iov_base=buffer; - iov[0].iov_len=bufferlen; - bzero(&msg,sizeof(msg)); - msg.msg_name = &recvaddr->store; - msg.msg_namelen = sizeof(recvaddr->store); - msg.msg_iov = &iov[0]; - msg.msg_iovlen = 1; - msg.msg_control = cmsgcmsg; - msg.msg_controllen = sizeof cmsgcmsg; - msg.msg_flags = 0; - - ssize_t len = recvmsg(sock,&msg,0); - if (len == -1 && errno != EAGAIN && errno != EWOULDBLOCK) - return WHYF_perror("recvmsg(%d,%p,0)", sock, &msg); - -#if 0 - DEBUGF(packetrx, "recvmsg returned %d (flags=%d, msg_controllen=%d)", (int) len, msg.msg_flags, (int)msg.msg_controllen); - if (IF_DEBUG(packetrx)) - dump("received data", buffer, len); -#endif - - if (len > 0) { - struct cmsghdr *cmsg; - for (cmsg = CMSG_FIRSTHDR(&msg); cmsg != NULL; cmsg = CMSG_NXTHDR(&msg, cmsg)) { - if ( cmsg->cmsg_level == IPPROTO_IP - && ((cmsg->cmsg_type == IP_RECVTTL) || (cmsg->cmsg_type == IP_TTL)) - && cmsg->cmsg_len - ) { - DEBUGF(packetrx, " TTL (%p) data location resolves to %p", ttl,CMSG_DATA(cmsg)); - if (CMSG_DATA(cmsg)) { - *ttl = *(unsigned char *) CMSG_DATA(cmsg); - DEBUGF(packetrx, " TTL of packet is %d", *ttl); - } - } else { - DEBUGF(packetrx, "I didn't expect to see level=%02x, type=%02x", cmsg->cmsg_level,cmsg->cmsg_type); - } - } - } - recvaddr->addrlen = msg.msg_namelen; - - return len; -} diff --git a/overlay_interface.c b/overlay_interface.c index 1455c05f..154da9d0 100644 --- a/overlay_interface.c +++ b/overlay_interface.c @@ -380,12 +380,8 @@ overlay_interface_read_any(struct sched_ent *alarm) /* Read only one UDP packet per call to share resources more fairly, and also enable stats to accurately count packets received */ - plen = recvwithttl(alarm->poll.fd, packet, sizeof(packet), &recvttl, &recvaddr); + plen = recv_message(alarm->poll.fd, &recvaddr, &recvttl, packet, sizeof(packet)); if (plen == -1) { - WHYF_perror("recvwithttl(%d,%p,%zu,&%d,%p(%s))", - alarm->poll.fd, packet, sizeof packet, recvttl, - &recvaddr, alloca_socket_address(&recvaddr) - ); unwatch(alarm); close(alarm->poll.fd); return; @@ -683,12 +679,8 @@ static void interface_read_dgram(struct overlay_interface *interface) /* Read only one UDP packet per call to share resources more fairly, and also enable stats to accurately count packets received */ int recvttl=1; - plen = recvwithttl(interface->alarm.poll.fd,packet, sizeof(packet), &recvttl, &recvaddr); + plen = recv_message(interface->alarm.poll.fd, &recvaddr, &recvttl, packet, sizeof(packet)); if (plen == -1) { - WHYF_perror("recvwithttl(%d,%p,%zu,&%d,%p(%s))", - interface->alarm.poll.fd, packet, sizeof packet, recvttl, - &recvaddr, alloca_socket_address(&recvaddr) - ); overlay_interface_close(interface); return; } diff --git a/overlay_mdp.c b/overlay_mdp.c index 5ed3fa02..5e390ba3 100644 --- a/overlay_mdp.c +++ b/overlay_mdp.c @@ -1717,18 +1717,10 @@ static void overlay_mdp_poll(struct sched_ent *alarm) { if (alarm->poll.revents & POLLIN) { unsigned char buffer[16384]; - int ttl; + int ttl = -1; struct socket_address client; client.addrlen = sizeof client.store; - - ttl=-1; - - ssize_t len = recvwithttl(alarm->poll.fd,buffer,sizeof(buffer),&ttl, &client); - if (len == -1) - WHYF_perror("recvwithttl(%d,%p,%zu,&%d,%p(%s))", - alarm->poll.fd, buffer, sizeof buffer, ttl, - &client, alloca_socket_address(&client) - ); + ssize_t len = recv_message(alarm->poll.fd, &client, &ttl, buffer, sizeof(buffer)); if ((size_t)len > 0) { if (client.addrlen <= sizeof(sa_family_t)) diff --git a/socket.c b/socket.c index 3099c461..ee031d4d 100644 --- a/socket.c +++ b/socket.c @@ -84,6 +84,7 @@ int _make_local_sockaddr(struct __sourceloc __whence, struct socket_address *add */ int real_sockaddr(const struct socket_address *src_addr, struct socket_address *dst_addr) { + DEBUGF2(io, verbose_io, "real_sockaddr(src_addr=%p %s, dst_addr=%p)", src_addr, alloca_socket_address(src_addr), dst_addr); assert(src_addr->addrlen > sizeof src_addr->local.sun_family); size_t src_path_len = src_addr->addrlen - sizeof src_addr->local.sun_family; if ( src_addr->addrlen >= sizeof src_addr->local.sun_family + 1 @@ -103,8 +104,10 @@ int real_sockaddr(const struct socket_address *src_addr, struct socket_address * ) { memcpy(dst_addr->local.sun_path, real_path, real_path_len); dst_addr->addrlen = real_path_len + sizeof dst_addr->local.sun_family; + DEBUGF2(io, verbose_io, " --> return %s", alloca_socket_address(dst_addr)); return 1; } + DEBUGF2(io, verbose_io, "real_path=%s", alloca_str_toprint(real_path)); } if (dst_addr != src_addr){ memcpy(&dst_addr->addr, &src_addr->addr, src_addr->addrlen); @@ -280,20 +283,61 @@ ssize_t _send_message(struct __sourceloc __whence, int fd, const struct socket_a ssize_t ret = sendmsg(fd, &hdr, 0); if (ret == -1 && errno != EAGAIN) WHYF_perror("sendmsg(%d,%s,%lu)", fd, alloca_socket_address(address), (unsigned long)address->addrlen); + DEBUGF(verbose_io, "sendmsg(%d, %s, %lu)", fd, alloca_socket_address(address), (unsigned long)address->addrlen); return ret; } -ssize_t _recv_message(struct __sourceloc __whence, int fd, struct socket_address *address, struct fragmented_data *data) +ssize_t _recv_message_frag(struct __sourceloc __whence, int fd, struct socket_address *address, int *ttl, struct fragmented_data *data) { - struct msghdr hdr={ - .msg_name=(void *)&address->addr, - .msg_namelen=address->addrlen, - .msg_iov=data->iov, - .msg_iovlen=data->fragment_count, + struct cmsghdr cmsgs[16]; + struct msghdr msg = { + .msg_name = (void *)&address->addr, + .msg_namelen = address->addrlen, + .msg_iov = data->iov, + .msg_iovlen = data->fragment_count, + .msg_control = cmsgs, + .msg_controllen = sizeof cmsgs, + .msg_flags = 0 }; - ssize_t ret = recvmsg(fd, &hdr, 0); - if (ret==-1) - WHYF_perror("recvmsg(%d,%s,%lu)", fd, alloca_socket_address(address), (unsigned long)address->addrlen); - address->addrlen = hdr.msg_namelen; + bzero(&address->addr, address->addrlen); + ssize_t ret = recvmsg(fd, &msg, 0); + if (ret == -1 && errno != EAGAIN && errno != EWOULDBLOCK) + WHYF_perror("recvmsg(%d,{name=%p,namelen=%u,iov=%s,control=%p,controllen=%u},0)", + fd, &address->addr, (unsigned) address->addrlen, + alloca_iovec(data->iov, data->fragment_count), + cmsgs, (unsigned) sizeof cmsgs); + address->addrlen = msg.msg_namelen; + if (ttl && ret > 0) { + struct cmsghdr *cmsg; + for (cmsg = CMSG_FIRSTHDR(&msg); cmsg != NULL; cmsg = CMSG_NXTHDR(&msg, cmsg)) { + if ( cmsg->cmsg_level == IPPROTO_IP + && ((cmsg->cmsg_type == IP_RECVTTL) || (cmsg->cmsg_type == IP_TTL)) + && cmsg->cmsg_len + ) { + DEBUGF(verbose_io, " TTL (%p) data location resolves to %p", ttl, CMSG_DATA(cmsg)); + if (CMSG_DATA(cmsg)) { + *ttl = *(unsigned char *) CMSG_DATA(cmsg); + DEBUGF(verbose_io, " TTL of packet is %d", *ttl); + } + } else { + DEBUGF(verbose_io, " unexpected level=%02x, type=%02x", cmsg->cmsg_level, cmsg->cmsg_type); + } + } + } + DEBUGF(verbose_io, "recvmsg(%d) -> %zd, flags=%x, address=%s ttl=%d", + fd, + ret, + msg.msg_flags, + alloca_socket_address(address), + ttl ? *ttl : -1); return ret; } + +ssize_t _recv_message(struct __sourceloc __whence, int fd, struct socket_address *address, int *ttl, unsigned char *buffer, size_t buflen) +{ + struct fragmented_data data; + data.fragment_count = 1; + data.iov[0].iov_base = buffer; + data.iov[0].iov_len = buflen; + return _recv_message_frag(__whence, fd, address, ttl, &data); +} diff --git a/socket.h b/socket.h index 51191361..b26e0360 100644 --- a/socket.h +++ b/socket.h @@ -81,11 +81,11 @@ int append_fragment(struct fragmented_data *data, const uint8_t *payload, size_t size_t copy_fragment(struct fragmented_data *src, uint8_t *dest, size_t length); ssize_t _send_message(struct __sourceloc, int fd, const struct socket_address *address, const struct fragmented_data *data); -ssize_t _recv_message(struct __sourceloc, int fd, struct socket_address *address, struct fragmented_data *data); +ssize_t _recv_message_frag(struct __sourceloc, int fd, struct socket_address *address, int *ttl, struct fragmented_data *data); +ssize_t _recv_message(struct __sourceloc __whence, int fd, struct socket_address *address, int *ttl, unsigned char *buffer, size_t buflen); -#define send_message(fd, address, data) _send_message(__WHENCE__, (fd), (address), (data)) -#define recv_message(fd, address, data) _recv_message(__WHENCE__, (fd), (address), (data)) - -ssize_t recvwithttl(int sock, unsigned char *buffer, size_t bufferlen, int *ttl, struct socket_address *recvaddr); +#define send_message(fd, address, data) _send_message(__WHENCE__, (fd), (address), (data)) +#define recv_message_frag(fd, address, ttl, data) _recv_message(__WHENCE__, (fd), (address), (ttl), (data)) +#define recv_message(fd, address, ttl, buf, len) _recv_message(__WHENCE__, (fd), (address), (ttl), (buf), (len)) #endif // __SERVAL_DNA___SOCKET_H diff --git a/sourcefiles.mk b/sourcefiles.mk index 1d5403be..659728e6 100644 --- a/sourcefiles.mk +++ b/sourcefiles.mk @@ -113,10 +113,9 @@ TEST_SOURCES = \ log_context.c \ log_stderr.c \ context1.c - + MDP_CLIENT_SOURCES = \ - mdp_client.c \ - mdp_net.c + mdp_client.c SIMULATOR_SOURCES = \ simulator.c