[PATCH v3] IPv6 timeserver for NTP

[Naveen Singh]

From: Naveen Singh <***@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;
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 *)&timeserver_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];

/*
* 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 *)&timeserver_addr)->sin_addr):(void *)&timeserver_addr.sin6_addr,
+ ipaddrstring,
+ 28);
+ }
+
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
- server, errno, strerror(errno));
+ ipaddrstring, errno, strerror(errno));

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);
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 *)&timeserver_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 *)&timeserver_addr)->sin_addr.s_addr;
+ src_ptr = (void *)&((struct sockaddr_in *)&sender_addr)->sin_addr.s_addr;
+ } else if(sender_addr.sin6_family == AF_INET6) {
+ size = 16;
+ addr_ptr = (void *)((struct sockaddr_in6 *)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8;
+ src_ptr = (void *)((struct sockaddr_in6 *)&sender_addr)->sin6_addr.__in6_u.__u6_addr8;
+ } 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 *)&timeserver_addr, ((struct sockaddr_in *)info->ai_addr), info->ai_addrlen);
+ ((struct sockaddr_in *)&timeserver_addr)->sin_port = htons(123);
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ } else if (family == AF_INET6) {
+ memcpy(&timeserver_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);
+ } 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, &timestamp,
@@ -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*)&timeserver_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);

[Naveen Singh]

Post by Naveen Singh
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;
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
*)&timeserver_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];
/*
@@ -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 *)&timeserver_addr)->sin_addr):(void
*)&timeserver_addr.sin6_addr,
+ ipaddrstring,
+ 28);
+ }
+
if (len < 0) {
connman_error("Time request for server %s failed (%d/%s)",
- server, errno, strerror(errno));
+ ipaddrstring, errno, strerror(errno));
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);
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 *)&timeserver_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
*)&timeserver_addr)->sin_addr.s_addr;
+ src_ptr = (void *)&((struct sockaddr_in
*)&sender_addr)->sin_addr.s_addr;
+ } else if(sender_addr.sin6_family == AF_INET6) {
+ size = 16;
+ addr_ptr = (void *)((struct sockaddr_in6
*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8;
+ src_ptr = (void *)((struct sockaddr_in6
*)&sender_addr)->sin6_addr.__in6_u.__u6_addr8;
+ } 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 *)&timeserver_addr, ((struct
sockaddr_in *)info->ai_addr), info->ai_addrlen);
+ ((struct sockaddr_in *)&timeserver_addr)->sin_port =
htons(123);
+ memset(&in4addr, 0, sizeof(in4addr));
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);
+ } else if (family == AF_INET6) {
+ memcpy(&timeserver_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);
+ } 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, &timestamp,
@@ -479,7 +547,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, (struct sockaddr*)&timeserver_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);
--
2.5.3

Hi Patrik and Jukka,
Everything done as per our last review. Tested it with an internet routable
IPv6 address and Ipv4 address as well as a fe80 ipv6 address. Only thing
that did not work for me was to use the ai_addr in struct addrinfo for bind
purpose (in start_ntp). Bind was failing all the time.

Regards
Naveen

[Patrik Flykt]

Hi,

Post by Naveen Singh
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.

Post by Naveen Singh
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 *)&timeserver_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.

Post by Naveen Singh
/*
@@ -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 *)&timeserver_addr)->sin_addr):(void *)&timeserver_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...

Post by Naveen Singh
+ }
+
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)), ...);

Post by Naveen Singh
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.

Post by Naveen Singh
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 *)&timeserver_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 *)&timeserver_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.

Post by Naveen Singh
+ } else if(sender_addr.sin6_family == AF_INET6) {

Space after 'if'

Post by Naveen Singh
+ size = 16;
+ addr_ptr = (void *)((struct sockaddr_in6 *)&timeserver_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.

Post by Naveen Singh
+ } 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 *)&timeserver_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.

Post by Naveen Singh
+ ((struct sockaddr_in *)&timeserver_addr)->sin_port = htons(123);

I didn't notice sin_port being used anywhere, so this is not needed?

Post by Naveen Singh
+ 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.

Post by Naveen Singh
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);

size is needed, but why the pointer?

Post by Naveen Singh
+ } else if (family == AF_INET6) {
+ memcpy(&timeserver_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.

Post by Naveen Singh
+ } 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, &timestamp,
@@ -479,7 +547,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, (struct sockaddr*)&timeserver_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

[Naveen Singh]

Hi Patrik,
Sorry for a very delayed reply on this thread. I do appreciate all your
feedback and I would send you the revised patch on Monday after testing it
for both IPv4 and IPv6 timeserver and all possible combinations.

Post by Naveen Singh
Hi,

Post by Naveen Singh
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

++++++++++++++++++++++++++++++++++++++++++++++----------------

Post by Naveen Singh
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.

Post by Naveen Singh
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);

Post by Naveen Singh
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

*)&timeserver_addr, timeout << 1);

Post by Naveen Singh
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)

Post by Naveen Singh
{
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.

Thanks for it. I made the change already.

Post by Naveen Singh

Post by Naveen Singh
/*
* At some point, we could specify the actual system precision
@@ -168,10 +170,14 @@ static void send_packet(int fd, const char

*server, uint32_t timeout)

Post by Naveen Singh
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)

Post by Naveen Singh
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 *)&timeserver_addr)->sin_addr):(void
*)&timeserver_addr.sin6_addr,

Post by Naveen Singh
+ 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...

I agree I have gotten rid of it and now I call inet_ntop twice as you
mentioned.

Post by Naveen Singh

Post by Naveen Singh
+ }
+
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)), ...);

Post by Naveen Singh
if (errno == ENETUNREACH)
__connman_timeserver_sync_next();
@@ -192,7 +206,7 @@ static void send_packet(int fd, const char *server,

uint32_t timeout)

Post by Naveen Singh
}
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.

I have done this.

Post by Naveen Singh

Post by Naveen Singh
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 *)&timeserver_addr,

NTP_SEND_TIMEOUT);

Post by Naveen Singh
return FALSE;
}
@@ -363,7 +377,7 @@ static gboolean received_data(GIOChannel *channel,

GIOCondition condition,

Post by Naveen Singh
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,

Post by Naveen Singh
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,

Post by Naveen Singh
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

*)&timeserver_addr)->sin_addr.s_addr;

Post by Naveen Singh
+ 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.

Post by Naveen Singh
+ } else if(sender_addr.sin6_family == AF_INET6) {

Space after 'if'

Post by Naveen Singh
+ size = 16;
+ addr_ptr = (void *)((struct sockaddr_in6

*)&timeserver_addr)->sin6_addr.__in6_u.__u6_addr8;

Post by Naveen Singh
+ src_ptr = (void *)((struct sockaddr_in6

*)&sender_addr)->sin6_addr.__in6_u.__u6_addr8;
As above, (void *) and ->sin6_addr.

Post by Naveen Singh
+ } 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,

Post by Naveen Singh
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) {

Post by Naveen Singh
- connman_error("Failed to bind time server socket");
- close(transmit_fd);
+ family = info->ai_family;
+ if (family == AF_INET) {
+ memcpy((struct sockaddr_in *)&timeserver_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.

Post by Naveen Singh
+ ((struct sockaddr_in *)&timeserver_addr)->sin_port =

htons(123);
I didn't notice sin_port being used anywhere, so this is not needed?

This is being used in current code so I decided to follow the current code.
In send_packet look for following line:
addr.sin_port = htons(123);

Post by Naveen Singh

Post by Naveen Singh
+ 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.

Post by Naveen Singh
+ in4addr.sin_family = family;
+ addr = (struct sockaddr *)&in4addr;
+ size = sizeof(in4addr);

size is needed, but why the pointer?

Post by Naveen Singh
+ } else if (family == AF_INET6) {
+ memcpy(&timeserver_addr, ((struct sockaddr_in6

*)info->ai_addr), info->ai_addrlen);

Post by Naveen Singh
+ 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.

Post by Naveen Singh
+ } else {
+ connman_error("Family is neither ipv4 nor ipv6");
return;
}
+ freeaddrinfo(info);
- if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos))

< 0) {

Post by Naveen Singh
- 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) {

Post by Naveen Singh
+ connman_error("Failed to set type of service

option");

Post by Naveen Singh
+ close(transmit_fd);
+ return;
+ }
}
if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, &timestamp,
@@ -479,7 +547,7 @@ static void start_ntp(char *server)
g_io_channel_unref(channel);
- send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
+ send_packet(transmit_fd, (struct sockaddr*)&timeserver_addr,

NTP_SEND_TIMEOUT);

Post by Naveen Singh
}
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

Thanks
Naveen

Post by Naveen Singh
_______________________________________________
connman mailing list
https://lists.connman.net/mailman/listinfo/connman

[Patrik Flykt]

Post by Naveen Singh

Post by Patrik Flykt
I didn't notice sin_port being used anywhere, so this is not needed?

This is being used in current code so I decided to follow the current
addr.sin_port = htons(123);

Mmmkay. But the port sent to is a constant (123), so it need not be
explicitely rembered. On the receiving end one needs to check the server
address so that some other whimsical device sending data to ConnMan can
be ignored.

Cheers,

Patrik

[Naveen Singh]

Post by Patrik Flykt

Post by Naveen Singh

Post by Patrik Flykt
I didn't notice sin_port being used anywhere, so this is not needed?

This is being used in current code so I decided to follow the current
addr.sin_port = htons(123);

Mmmkay. But the port sent to is a constant (123), so it need not be
explicitely rembered. On the receiving end one needs to check the server
address so that some other whimsical device sending data to ConnMan can
be ignored.

Yes the receive path does that check anyway.
When we call send_packet from start_ntp we pass the sockaddr as
timeserver_addr (which is used for sending ntp request packet) I
initialized the port number. Please have a look in the patch that I am
sending

Post by Patrik Flykt
Cheers,
Patrik
_______________________________________________
connman mailing list
https://lists.connman.net/mailman/listinfo/connman