Hi, On Tue, 2015-10-06 at 23:50 -0700, Naveen Singh wrote: > From: Naveen Singh <nav...@nestlabs.com> > > Current NTP code is written with an assumption that timeserver is > always an IPv4 address. If there is an IPv6 timeserver then the socket > operation would fail with error as Permission denied (13). This change in > ntp.c ensures that code works fine with both IPv4 and IPv6 address. > --- > src/ntp.c | 139 > ++++++++++++++++++++++++++++++++++++++++++++++---------------- > 1 file changed, 103 insertions(+), 36 deletions(-) > > diff --git a/src/ntp.c b/src/ntp.c > index 2c313a4..7858633 100644 > --- a/src/ntp.c > +++ b/src/ntp.c > @@ -34,6 +34,7 @@ > #include <netinet/in.h> > #include <netinet/ip.h> > #include <arpa/inet.h> > +#include <netdb.h> > > #include <glib.h> > > @@ -117,12 +118,12 @@ static struct timespec mtx_time; > static int transmit_fd = 0; > > static char *timeserver = NULL; > -static struct sockaddr_in timeserver_addr; > +static struct sockaddr_in6 timeserver_addr;
Yes, this should be large enough. > static gint poll_id = 0; > static gint timeout_id = 0; > static guint retries = 0; > > -static void send_packet(int fd, const char *server, uint32_t timeout); > +static void send_packet(int fd, struct sockaddr *server, uint32_t timeout); > > static void next_server(void) > { > @@ -143,17 +144,18 @@ static gboolean send_timeout(gpointer user_data) > if (retries++ == NTP_SEND_RETRIES) > next_server(); > else > - send_packet(transmit_fd, timeserver, timeout << 1); > + send_packet(transmit_fd, (struct sockaddr *)×erver_addr, > timeout << 1); > > return FALSE; > } > > -static void send_packet(int fd, const char *server, uint32_t timeout) > +static void send_packet(int fd, struct sockaddr *server, uint32_t timeout) > { > struct ntp_msg msg; > - struct sockaddr_in addr; > struct timeval transmit_timeval; > ssize_t len; > + int size; > + char ipaddrstring[28]; The size is not correct. Use INET6_ADDRSTRLEN + 1 as it wants to be null terminated also. > > /* > * At some point, we could specify the actual system precision with: > @@ -168,10 +170,14 @@ static void send_packet(int fd, const char *server, > uint32_t timeout) > msg.poll = 10; // max > msg.precision = NTP_PRECISION_S; > > - memset(&addr, 0, sizeof(addr)); > - addr.sin_family = AF_INET; > - addr.sin_port = htons(123); > - addr.sin_addr.s_addr = inet_addr(server); > + if (server->sa_family == AF_INET) { > + size = sizeof(struct sockaddr_in); > + } else if (server->sa_family == AF_INET6){ > + size = sizeof(struct sockaddr_in6); > + } else { > + connman_error("Family is neither ipv4 nor ipv6"); > + return; > + } > > gettimeofday(&transmit_timeval, NULL); > clock_gettime(CLOCK_MONOTONIC, &mtx_time); > @@ -180,10 +186,18 @@ static void send_packet(int fd, const char *server, > uint32_t timeout) > msg.xmttime.fraction = htonl(transmit_timeval.tv_usec * 1000); > > len = sendto(fd, &msg, sizeof(msg), MSG_DONTWAIT, > - &addr, sizeof(addr)); > + server, size); > + > + if ((len < 0) || (len != sizeof(msg))) { > + inet_ntop(server->sa_family, > + (server->sa_family == AF_INET)?(void *)&(((struct sockaddr_in > *)×erver_addr)->sin_addr):(void *)×erver_addr.sin6_addr, > + ipaddrstring, > + 28); This very complicated. In the previous block server->sa_family is already investigated, why not record the address of sin_addr or sin6_addr into a pointer already there? inet_ntop wants a void * anyway. inet_ntop is not needed right away, because... > + } > + > if (len < 0) { > connman_error("Time request for server %s failed (%d/%s)", > - server, errno, strerror(errno)); > + ipaddrstring, errno, strerror(errno)); ...it can be used here in the log print, as it returns a non-null pointer as well. So we would have connman_error(..., inet_ntop(server->sa_family, addr, &ipaddrstr, sizeof(ipaddrstr)), ...); > > if (errno == ENETUNREACH) > __connman_timeserver_sync_next(); > @@ -192,7 +206,7 @@ static void send_packet(int fd, const char *server, > uint32_t timeout) > } > > if (len != sizeof(msg)) { > - connman_error("Broken time request for server %s", server); > + connman_error("Broken time request for server %s", > ipaddrstring); Same here. Yes, it duplicates the print but makes the logic a bit easier to follow. > return; > } > > @@ -213,7 +227,7 @@ static gboolean next_poll(gpointer user_data) > if (!timeserver || transmit_fd == 0) > return FALSE; > > - send_packet(transmit_fd, timeserver, NTP_SEND_TIMEOUT); > + send_packet(transmit_fd, (struct sockaddr *)×erver_addr, > NTP_SEND_TIMEOUT); > > return FALSE; > } > @@ -363,7 +377,7 @@ static gboolean received_data(GIOChannel *channel, > GIOCondition condition, > gpointer user_data) > { > unsigned char buf[128]; > - struct sockaddr_in sender_addr; > + struct sockaddr_in6 sender_addr; > struct msghdr msg; > struct iovec iov; > struct cmsghdr *cmsg; > @@ -372,6 +386,9 @@ static gboolean received_data(GIOChannel *channel, > GIOCondition condition, > char aux[128]; > ssize_t len; > int fd; > + int size; > + void * addr_ptr; > + void * src_ptr; > > if (condition & (G_IO_HUP | G_IO_ERR | G_IO_NVAL)) { > connman_error("Problem with timer server channel"); > @@ -396,8 +413,20 @@ static gboolean received_data(GIOChannel *channel, > GIOCondition condition, > if (len < 0) > return TRUE; > > - if (timeserver_addr.sin_addr.s_addr != sender_addr.sin_addr.s_addr) > - /* only accept messages from the timeserver */ > + if (sender_addr.sin6_family == AF_INET) { > + size = 4; > + addr_ptr = (void *)&((struct sockaddr_in > *)×erver_addr)->sin_addr.s_addr; > + src_ptr = (void *)&((struct sockaddr_in > *)&sender_addr)->sin_addr.s_addr; The cast to (void *) doesn't seem to be necessary and ->sin_addr should be enough as well. > + } else if(sender_addr.sin6_family == AF_INET6) { Space after 'if' > + size = 16; > + addr_ptr = (void *)((struct sockaddr_in6 > *)×erver_addr)->sin6_addr.__in6_u.__u6_addr8; > + src_ptr = (void *)((struct sockaddr_in6 > *)&sender_addr)->sin6_addr.__in6_u.__u6_addr8; As above, (void *) and ->sin6_addr. > + } else { > + connman_error("Not a valid family type"); > + return TRUE; > + } > + > + if(memcmp(addr_ptr, src_ptr, size) != 0) > return TRUE; > > tv = NULL; > @@ -422,36 +451,75 @@ static gboolean received_data(GIOChannel *channel, > GIOCondition condition, > static void start_ntp(char *server) > { > GIOChannel *channel; > - struct sockaddr_in addr; > + struct addrinfo hint; > + struct addrinfo *info; > + struct sockaddr * addr; > + struct sockaddr_in in4addr; > + struct sockaddr_in6 in6addr; > + int size; > + int family; > int tos = IPTOS_LOWDELAY, timestamp = 1; > + int ret; > > if (!server) > return; > > - DBG("server %s", server); > - > - if (channel_watch > 0) > - goto send; > + memset(&hint, 0, sizeof(hint)); > + hint.ai_family = AF_UNSPEC; > + hint.ai_socktype = SOCK_DGRAM; > + hint.ai_flags = AI_NUMERICHOST | AI_PASSIVE; > + ret = getaddrinfo(server, NULL, &hint, &info); > > - transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0); > - if (transmit_fd < 0) { > - connman_error("Failed to open time server socket"); > + if (ret) { > + connman_error("cannot get server info"); > return; > } > > - memset(&addr, 0, sizeof(addr)); > - addr.sin_family = AF_INET; > - > - if (bind(transmit_fd, (struct sockaddr *) &addr, sizeof(addr)) < 0) { > - connman_error("Failed to bind time server socket"); > - close(transmit_fd); > + family = info->ai_family; > + if (family == AF_INET) { > + memcpy((struct sockaddr_in *)×erver_addr, ((struct > sockaddr_in *)info->ai_addr), info->ai_addrlen); memcopy expects void * so the cast to (struct sockaddr_in *) seems totally unnecessary in both places. Do one common memcopy after setting the needed variable values outside of these 'if's. > + ((struct sockaddr_in *)×erver_addr)->sin_port = htons(123); I didn't notice sin_port being used anywhere, so this is not needed? > + memset(&in4addr, 0, sizeof(in4addr)); Set addr's type to struct sockaddr_in6. Then the memset can be done above as before and only once. > + in4addr.sin_family = family; > + addr = (struct sockaddr *)&in4addr; > + size = sizeof(in4addr); size is needed, but why the pointer? > + } else if (family == AF_INET6) { > + memcpy(×erver_addr, ((struct sockaddr_in6 > *)info->ai_addr), info->ai_addrlen); > + timeserver_addr.sin6_port = htons(123); > + memset(&in6addr, 0, sizeof(in6addr)); > + in6addr.sin6_family = family; > + addr = (struct sockaddr *)&in6addr; > + size = sizeof(in6addr); Same comments apply as in the other branch. > + } else { > + connman_error("Family is neither ipv4 nor ipv6"); > return; > } > + freeaddrinfo(info); > > - if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos)) < 0) > { > - connman_error("Failed to set type of service option"); > - close(transmit_fd); > - return; > + DBG("server %s family %d", server, family); > + > + if (channel_watch > 0) > + goto send; > + > + transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0); > + > + if (transmit_fd <= 0) { > + connman_error("Failed to open time server socket"); > + return; > + } > + > + if (bind(transmit_fd, (struct sockaddr *) addr, size) < 0) { > + connman_error("Failed to bind time server socket"); > + close(transmit_fd); > + return; > + } > + > + if (family == AF_INET) { > + if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, > sizeof(tos)) < 0) { > + connman_error("Failed to set type of service option"); > + close(transmit_fd); > + return; > + } > } > > if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, ×tamp, > @@ -479,7 +547,7 @@ static void start_ntp(char *server) > g_io_channel_unref(channel); > > send: > - send_packet(transmit_fd, server, NTP_SEND_TIMEOUT); > + send_packet(transmit_fd, (struct sockaddr*)×erver_addr, > NTP_SEND_TIMEOUT); > } > > int __connman_ntp_start(char *server) > @@ -493,7 +561,6 @@ int __connman_ntp_start(char *server) > g_free(timeserver); > > timeserver = g_strdup(server); > - timeserver_addr.sin_addr.s_addr = inet_addr(server); > > start_ntp(timeserver); > Cheers, Patrik _______________________________________________ connman mailing list connman@connman.net https://lists.connman.net/mailman/listinfo/connman