Hi,
On Thu, Sep 24, 2015 at 1:14 AM, Patrik Flykt <patrik.fl...@linux.intel.com>
wrote:
>
> Hi,
>
> On Wed, 2015-09-23 at 21:22 -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 | 144
> ++++++++++++++++++++++++++++++++++++++++++++++++--------------
> > 1 file changed, 113 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/ntp.c b/src/ntp.c
> > index 2c313a4..5c135ce 100644
> > --- a/src/ntp.c
> > +++ b/src/ntp.c
> > @@ -18,7 +18,6 @@
> > * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> 02110-1301 USA
> > *
> > */
> > -
> > #ifdef HAVE_CONFIG_H
> > #include <config.h>
> > #endif
> > @@ -34,6 +33,7 @@
> > #include <netinet/in.h>
> > #include <netinet/ip.h>
> > #include <arpa/inet.h>
> > +#include <netdb.h>
> >
> > #include <glib.h>
> >
> > @@ -117,12 +117,12 @@ static struct timespec mtx_time;
> > static int transmit_fd = 0;
> >
> > static char *timeserver = NULL;
> > -static struct sockaddr_in timeserver_addr;
> > +static struct sockaddr_storage 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, const char *server, uint32_t timeout,
> int family);
> >
> > static void next_server(void)
> > {
> > @@ -143,17 +143,21 @@ 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, timeserver, timeout << 1,
> timeserver_addr.ss_family);
> >
> > return FALSE;
> > }
> >
> > -static void send_packet(int fd, const char *server, uint32_t timeout)
> > +static void send_packet(int fd, const char *server, uint32_t timeout,
> int family)
> > {
> > struct ntp_msg msg;
> > - struct sockaddr_in addr;
> > + struct sockaddr_in in4addr;
> > + struct sockaddr_in6 in6addr;
> > + struct sockaddr * addr;
> > struct timeval transmit_timeval;
> > ssize_t len;
> > + int size;
> > + unsigned char * addrptr;
> >
> > /*
> > * At some point, we could specify the actual system precision
> with:
> > @@ -168,10 +172,29 @@ 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 (family == AF_INET) {
> > + memset(&in4addr, 0, sizeof(in4addr));
> > + in4addr.sin_family = AF_INET;
> > + in4addr.sin_port = htons(123);
> > + size = sizeof(in4addr);
> > + addrptr = (unsigned char *)&in4addr.sin_addr.s_addr;
> > + addr = (struct sockaddr *)&in4addr;
> > + } else if (family == AF_INET6){
> > + memset(&in6addr, 0, sizeof(in6addr));
> > + in6addr.sin6_family = AF_INET6;
> > + in6addr.sin6_port = htons(123);
> > + size = sizeof(in6addr);
> > + addrptr = in6addr.sin6_addr.__in6_u.__u6_addr8;
> > + addr = (struct sockaddr *)&in6addr;
> > + } else {
> > + DBG("wrong family type");
> > + return;
> > + }
> > + if (inet_pton(family, server, addrptr) == 0)
> > + {
> > + DBG("cannot convert ip address string");
> > + return;
> > + }
> >
> > gettimeofday(&transmit_timeval, NULL);
> > clock_gettime(CLOCK_MONOTONIC, &mtx_time);
> > @@ -180,7 +203,7 @@ 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));
> > + addr, size);
> > if (len < 0) {
> > connman_error("Time request for server %s failed (%d/%s)",
> > server, errno, strerror(errno));
> > @@ -213,7 +236,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, timeserver, NTP_SEND_TIMEOUT,
> timeserver_addr.ss_family);
> >
> > return FALSE;
> > }
> > @@ -363,7 +386,7 @@ static gboolean received_data(GIOChannel *channel,
> GIOCondition condition,
> > gpointer user_data)
> > {
> > unsigned char buf[128];
> > - struct sockaddr_in sender_addr;
> > + struct sockaddr_storage sender_addr;
> > struct msghdr msg;
> > struct iovec iov;
> > struct cmsghdr *cmsg;
> > @@ -396,9 +419,18 @@ 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.ss_family == AF_INET) {
> > + if (((struct sockaddr_in
> *)×erver_addr)->sin_addr.s_addr != ((struct sockaddr_in
> *)&sender_addr)->sin_addr.s_addr)
> > + return TRUE;
> > + } else if(sender_addr.ss_family == AF_INET6) {
> > + if (memcmp(((struct sockaddr_in6
> *)×erver_addr)->sin6_addr.__in6_u.__u6_addr8,
> > + ((struct sockaddr_in6
> *)&sender_addr)->sin6_addr.__in6_u.__u6_addr8,
> > + 16) != 0)
> > + return TRUE;
> > + } else {
> > + connman_error("Not a valid family type");
> > return TRUE;
> > + }
> >
> > tv = NULL;
> > clock_gettime(CLOCK_MONOTONIC, &mrx_time);
> > @@ -419,40 +451,58 @@ static gboolean received_data(GIOChannel *channel,
> GIOCondition condition,
> > return TRUE;
> > }
> >
> > -static void start_ntp(char *server)
> > +static void start_ntp(char *server, int family)
> > {
> > GIOChannel *channel;
> > - struct sockaddr_in addr;
> > + struct sockaddr * addr;
> > + struct sockaddr_in in4addr;
> > + struct sockaddr_in6 in6addr;
> > int tos = IPTOS_LOWDELAY, timestamp = 1;
> > + int size;
> >
> > if (!server)
> > return;
> >
> > - DBG("server %s", server);
> > + DBG("server %s family %d", server, family);
> >
> > if (channel_watch > 0)
> > goto send;
> >
> > - transmit_fd = socket(PF_INET, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> > - if (transmit_fd < 0) {
> > + transmit_fd = socket(family, SOCK_DGRAM | SOCK_CLOEXEC, 0);
> > + if (transmit_fd <= 0) {
> > connman_error("Failed to open time server socket");
> > 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);
> > + if (family == AF_INET) {
> > + memset(&in4addr, 0, sizeof(in4addr));
> > + in4addr.sin_family = family;
> > + addr = (struct sockaddr *)&in4addr;
> > + size = sizeof(in4addr);
> > + } else if (family == AF_INET6) {
> > + memset(&in6addr, 0, sizeof(in6addr));
> > + in6addr.sin6_family = family;
> > + addr = (struct sockaddr *)&in6addr;
> > + size = sizeof(in6addr);
> > + } else {
> > + connman_error("Family not correct");
> > return;
> > }
> >
> > - if (setsockopt(transmit_fd, IPPROTO_IP, IP_TOS, &tos, sizeof(tos))
> < 0) {
> > - connman_error("Failed to set type of service option");
> > + 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;
> > + }
> > + } else if (family == AF_INET6) {
> > + //TO Do
> > + }
> >
> > if (setsockopt(transmit_fd, SOL_SOCKET, SO_TIMESTAMP, ×tamp,
> > sizeof(timestamp)) < 0) {
> > @@ -479,12 +529,16 @@ static void start_ntp(char *server)
> > g_io_channel_unref(channel);
> >
> > send:
> > - send_packet(transmit_fd, server, NTP_SEND_TIMEOUT);
> > + send_packet(transmit_fd, server, NTP_SEND_TIMEOUT, family);
> > }
> >
> > int __connman_ntp_start(char *server)
> > {
> > DBG("%s", server);
> > + struct addrinfo hint;
> > + struct addrinfo *info;
> > + int family;
> > + int ret;
> >
> > if (!server)
> > return -EINVAL;
> > @@ -492,10 +546,38 @@ int __connman_ntp_start(char *server)
> > if (timeserver)
> > g_free(timeserver);
> >
> > + memset(&hint, 0, sizeof(hint));
> > + hint.ai_family = AF_UNSPEC;
> > + hint.ai_flags = AI_NUMERICHOST;
> > + ret = getaddrinfo(server, NULL, &hint, &info);
>
> This functionality already exists in the form of
> _connman_inet_check_ipaddress(). It takes the server as an argument and
> returns the matching AF_ value if it's an IP address. With this in mind,
> the check itself should be done in start_ntp() to reduce extra
> parameters carried between the functions. That way it shrinks down the
> patch to the bare minimum changes needed.
>
The only reason i was calling this function here is to get family of the IP
address (as argument passed to this function is just a character string).
This family is needed for inet_pton so that we can convert the charater
string into the appropriate IP address struct and store it in
timeserver_addr. Since we got the family here I just passed it to start_ntp
so we do not have to do this again.
>
> > +
> > + if (ret) {
> > + connman_error("cannot get the server info");
> > + return -1;
> > + }
> > +
> > + family = info->ai_family;
> > + if (family == AF_INET) {
> > + if (inet_pton(family, server, &(((struct sockaddr_in
> *)×erver_addr)->sin_addr.s_addr)) == 0) {
> > + connman_error("cannot convert ip address string");
> > + return -1;
> > + }
> > + } else if (family == AF_INET6) {
> > + if (inet_pton(family, server, ((struct sockaddr_in6
> *)×erver_addr)->sin6_addr.__in6_u.__u6_addr8) == 0) {
> > + connman_error("cannot convert ipv6 address
> string");
> > + return -1;
> > + }
> > + } else {
> > + connman_error("Neither IPv4 nor IPv6 type");
> > + return -1;
>
> This check has already happened in src/service.c, set_property() and
> thus need not be re-done here. It is not done in src/clock.c
> set_property(), neither for the main.conf FallbackTimeservers, so those
> parts need a new patch each.
>
This check here is to just store the appropriate IP address into
timerserver_addr and knowing family helps us in typecasting timeserver_addr
to appropriate structure.
>
> > + }
> > +
> > + timeserver_addr.ss_family = family;
> > timeserver = g_strdup(server);
> > - timeserver_addr.sin_addr.s_addr = inet_addr(server);
> >
> > - start_ntp(timeserver);
> > + freeaddrinfo(info);
> > +
> > + start_ntp(timeserver, family);
> >
> > return 0;
> > }
>
> Cheers,
>
> Patrik
>
>
> _______________________________________________
> connman mailing list
> connman@connman.net
> https://lists.connman.net/mailman/listinfo/connman
>
_______________________________________________
connman mailing list
connman@connman.net
https://lists.connman.net/mailman/listinfo/connman
