Re: [PATCH v3] IPv6 timeserver for NTP

[Naveen Singh]

On Mon, Oct 26, 2015 at 12:46 AM, Patrik Flykt  wrote:

> On Fri, 2015-10-23 at 06:28 -0700, Naveen Singh wrote:
> > > 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);
>
> 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

>
> 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

Re: [PATCH v3] IPv6 timeserver for NTP

[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.



On Thu, Oct 8, 2015 at 5:22 AM, Patrik Flykt 
wrote:

>
> Hi,
>
> On Tue, 2015-10-06 at 23:50 -0700, Naveen Singh wrote:
> > From: 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 
> >  #include 
> >  #include 
> > +#include 
> >
> >  #include 
> >
> > @@ -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
> *)_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.
>

Thanks for it. I made the change already.

>
> >
> >   /*
> >* 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(, 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(_timeval, NULL);
> >   clock_gettime(CLOCK_MONOTONIC, _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, , sizeof(msg), MSG_DONTWAIT,
> > - , sizeof(addr));
> > + server, size);
> > +
> > + if ((len < 0) || (len != sizeof(msg))) {
> > + inet_ntop(server->sa_family,
> > + (server->sa_family == AF_INET)?(void *)&(((struct
> sockaddr_in *)_addr)->sin_addr):(void
> *)_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...
>

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

>
> > + }
> > +
> >   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, , sizeof(ipaddrstr)), ...);
>
> >
> >   if (errno == ENETUNREACH)
> >   __connman_timeserver_sync_next();
> > @@ -192,7 +206,7 @@ static void send_packet(int fd, const 

Re: [PATCH v3] IPv6 timeserver for NTP

[Patrik Flykt]

Hi,

On Tue, 2015-10-06 at 23:50 -0700, Naveen Singh wrote:
> From: 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 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -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 *)_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(, 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(_timeval, NULL);
>   clock_gettime(CLOCK_MONOTONIC, _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, , sizeof(msg), MSG_DONTWAIT,
> - , sizeof(addr));
> + server, size);
> +
> + if ((len < 0) || (len != sizeof(msg))) {
> + inet_ntop(server->sa_family,
> + (server->sa_family == AF_INET)?(void *)&(((struct sockaddr_in 
> *)_addr)->sin_addr):(void *)_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, , 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 *)_addr, 
> 

Re: [PATCH v3] IPv6 timeserver for NTP

[Naveen Singh]

On Tue, Oct 6, 2015 at 11:50 PM, Naveen Singh 
wrote:

> From: 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 
>  #include 
>  #include 
> +#include 
>
>  #include 
>
> @@ -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
> *)_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(, 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(_timeval, NULL);
> clock_gettime(CLOCK_MONOTONIC, _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, , sizeof(msg), MSG_DONTWAIT,
> -   , sizeof(addr));
> +   server, size);
> +
> +   if ((len < 0) || (len != sizeof(msg))) {
> +   inet_ntop(server->sa_family,
> +   (server->sa_family == AF_INET)?(void *)&(((struct
> sockaddr_in *)_addr)->sin_addr):(void
> *)_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 *)_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];
>