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
