[Git][NTPsec/ntpsec][master] End type-punning around the refid field. A pure refactoring step.

Eric S. Raymond gitlab at mg.gitlab.com
Mon Jan 28 14:08:30 UTC 2019


Eric S. Raymond pushed to branch master at NTPsec / ntpsec


Commits:
ebeb660e by Eric S. Raymond at 2019-01-28T14:06:52Z
End type-punning around the refid field. A pure refactoring step.

Before this patch refids were sometimes represented as a uint32_t and sometimes
as a char array of lenth REFIDLEN=4. Fixing this is a step towards unifying
struct pkt with struct parsed_pkt so we can do extension handling.

- - - - -


12 changed files:

- include/ntp.h
- include/ntp_endian.h
- include/ntp_refclock.h
- include/ntpd.h
- libntp/ntp_endian.c
- ntpd/ntp_control.c
- ntpd/ntp_io.c
- ntpd/ntp_proto.c
- ntpd/ntp_refclock.c
- ntpd/ntp_timer.c
- ntpd/refclock_generic.c
- ntpd/refclock_modem.c


Changes:

=====================================
include/ntp.h
=====================================
@@ -109,6 +109,7 @@ extern uint64_t ntp_random64 (void);
 #define LOGTOD(a)	ldexp(1., (int)(a)) /* log2 to double */
 #define ULOGTOD(a)	ldexp(1., (int)(a)) /* ulog2 to double */
 
+#define REFIDLEN	sizeof(uint32_t)	/* size of IPv4 network addr */
 
 /*
  * The netendpt structure is used to hold the addresses and socket
@@ -126,7 +127,7 @@ typedef struct netendpt {
 	unsigned short	family;		/* AF_INET/AF_INET6 */
 	unsigned short	phase;		/* phase in update cycle */
 	uint32_t	flags;		/* interface flags */
-	uint32_t	addr_refid;	/* IPv4 addr or IPv6 hash */
+	char		addr_refid[REFIDLEN];	/* IPv4 addr or IPv6 hash */
 	unsigned long	starttime;	/* current_time at creation */
 	volatile long	received;	/* number of incoming packets */
 	long		sent;		/* number of outgoing packets */
@@ -255,7 +256,7 @@ struct peer {
 	int8_t	precision;	/* remote clock precision */
 	double	rootdelay;	/* roundtrip delay to primary source */
 	double	rootdisp;	/* dispersion to primary source */
-	uint32_t	refid;	/* remote reference ID */
+	char	refid[REFIDLEN];/* remote reference ID */
 	l_fp	reftime;	/* update epoch */
 
 #define clear_to_zero status
@@ -373,14 +374,6 @@ struct peer {
 #define	FLAG_DNS	0x0800u	/* needs DNS lookup */
 #define FLAG_TSTAMP_PPS	0x4cd000u	/* PPS source provides absolute timestamp */
 
-
-/*
- * It's ugly that refid is sometimes treated as a  uint32_t and sometimes
- * as a string; that should be fixed. Using this in memcpy() at least
- * contains the problem.
- */
-#define REFIDLEN	sizeof(uint32_t)
-
 /* This is the new, sane way of representing packets. All fields are
    in host byte order, and the fixed-point time fields are just integers,
    with uints of 2^-16 or 2^-32 seconds as appropriate. */
@@ -420,7 +413,7 @@ struct pkt {
 	int8_t	precision;	/* peer clock precision */
 	u_fp	rootdelay;	/* roundtrip delay to primary source */
 	u_fp	rootdisp;	/* dispersion to primary source*/
-	uint32_t	refid;		/* reference id */
+	char	refid[REFIDLEN];	/* reference id */
 	l_fp_w	reftime;	/* last update time */
 	l_fp_w	org;		/* originate time stamp */
 	l_fp_w	rec;		/* receive time stamp */


=====================================
include/ntp_endian.h
=====================================
@@ -13,6 +13,7 @@
 
 uint16_t ntp_be16dec(const void *buf) __attribute__((pure));
 uint32_t ntp_be32dec(const void *buf) __attribute__((pure));
+void ntp_be32enc(void *buf, uint32_t x);
 uint64_t ntp_be64dec(const void *buf) __attribute__((pure));
 
 #endif


=====================================
include/ntp_refclock.h
=====================================
@@ -137,7 +137,7 @@ struct refclockproc {
 	double	fudgetime1;	/* fudge time1 */
 	double	fudgetime2;	/* fudge time2 */
 	uint8_t	stratum;	/* server stratum */
-	uint32_t	refid;		/* reference identifier */
+	char	refid[REFIDLEN]; /* reference identifier */
 	uint8_t	sloppyclockflag; /* driver options */
 
 	/*


=====================================
include/ntpd.h
=====================================
@@ -337,7 +337,7 @@ struct system_variables {
     double	sys_rootdelay;		/* roundtrip delay to primary source */
     double	sys_rootdisp;		/* dispersion to primary source */
     double	sys_rootdist;		/* distance to primary source */
-    uint32_t	sys_refid;		/* reference id */
+    char	sys_refid[REFIDLEN];	/* reference id */
     l_fp	sys_reftime;		/* last update time */
     struct peer *sys_peer;		/* current peer */
 };


=====================================
libntp/ntp_endian.c
=====================================
@@ -24,6 +24,14 @@ uint32_t ntp_be32dec(const void *buf) {
 	    (uint32_t)(b[3]);
 }
 
+void ntp_be32enc(void *buf, uint32_t x) {
+	uint8_t *b = (uint8_t*)buf;
+	b[0] = (x >> 24) & 0xff;
+	b[1] = (x >> 16) & 0xff;
+	b[2] = (x >> 8) & 0xff;
+	b[3] = x & 0xff;
+}
+
 uint64_t ntp_be64dec(const void *buf) {
 	const uint8_t *b = (const uint8_t*)buf;
 	return ((uint64_t)(b[0]) << 56) +


=====================================
ntpd/ntp_control.c
=====================================
@@ -66,9 +66,9 @@ static	void	ctl_putuint	(const char *, uint64_t);
 static	void	ctl_puthex	(const char *, uint64_t);
 static	void	ctl_putint	(const char *, long);
 static	void	ctl_putts	(const char *, l_fp *);
-static	void	ctl_putadr	(const char *, uint32_t,
+static	void	ctl_putadr	(const char *, char[REFIDLEN],
 				 sockaddr_u *);
-static	void	ctl_putrefid	(const char *, uint32_t);
+static	void	ctl_putrefid	(const char *, char *);
 static	void	ctl_putarray	(const char *, double *, int);
 static	void	ctl_putsys	(int);
 static	void	ctl_putpeer	(int, struct peer *);
@@ -701,7 +701,7 @@ init_control(void)
 }
 
 /*
- * unmarshall_ntp_control - unmarshall data stream into a ntp_sontrol struct
+ * unmarshall_ntp_control - unmarshall data stream into a ntp_control struct
  */
 void
 unmarshall_ntp_control(struct ntp_control *pkt, struct recvbuf *rbufp)
@@ -1368,7 +1368,7 @@ ctl_putts(
 static void
 ctl_putadr(
 	const char *tag,
-	uint32_t addr32,
+	char refid[REFIDLEN],
 	sockaddr_u *addr
 	)
 {
@@ -1383,9 +1383,8 @@ ctl_putadr(
 
 	*cp++ = '=';
 	if (NULL == addr) {
-		struct in_addr in4;
-		in4.s_addr = addr32;
-		cq = inet_ntoa(in4);
+		/* refid is an IPv4 address in binary format, just copy it */
+		cq = refid;
 	}
 	else
 		cq = socktoa(addr);
@@ -1397,12 +1396,12 @@ ctl_putadr(
 
 
 /*
- * ctl_putrefid - send a uint32_t refid as printable text
+ * ctl_putrefid - send a refid as printable text
  */
 static void
 ctl_putrefid(
 	const char *	tag,
-	uint32_t		refid
+	char		refid[REFIDLEN]
 	)
 {
 	char	output[16];
@@ -1422,8 +1421,8 @@ ctl_putrefid(
 	}
 	if (!(optr < oplim))
 		return;
-	iptr = (char *)&refid;
-	iplim = iptr + sizeof(refid);
+	iptr = refid;
+	iplim = iptr + REFIDLEN;
 	for ( ; optr < oplim && iptr < iplim && '\0' != *iptr;
 	     iptr++, optr++)
 		if (isprint((int)*iptr))
@@ -2435,13 +2434,15 @@ ctl_putclock(
 		break;
 
 	case CC_FUDGEVAL2:
+		/*
+	         * ESR: NTP Classic sometimes shipped this as a refid string,
+		 * which seems wrong. If you need to look at that code,
+		 * any version of NTP Classic or NTPsec before January
+		 * 2019 has it that way.
+	         */
 		if (mustput || (pcs->haveflags & CLK_HAVEVAL2)) {
-			if (pcs->fudgeval1 > 1)
-				ctl_putadr(clock_var[id].text,
-					   pcs->fudgeval2, NULL);
-			else
-				ctl_putrefid(clock_var[id].text,
-					     pcs->fudgeval2);
+			ctl_putint(clock_var[id].text,
+				   pcs->fudgeval2);
 		}
 		break;
 


=====================================
ntpd/ntp_io.c
=====================================
@@ -19,6 +19,7 @@
 #include "ntp_refclock.h"
 #include "ntp_stdlib.h"
 #include "ntp_assert.h"
+#include "ntp_endian.h"
 #include "ntp_dns.h"
 #include "timespecops.h"
 
@@ -393,7 +394,9 @@ interface_dump(const endpt *itf)
 	sockaddr_dump(&itf->mask);
 	printf("name = %s\n", itf->name);
 	printf("flags = 0x%08x\n", itf->flags);
-	printf("addr_refid = %08x\n", itf->addr_refid);
+	printf("addr_refid = %02x%02x%02x%02x\n",
+	       itf->addr_refid[0], itf->addr_refid[1],
+	       itf->addr_refid[2], itf->addr_refid[3]);
 	printf("received = %ld\n", itf->received);
 	printf("sent = %ld\n", itf->sent);
 	printf("notsent = %ld\n", itf->notsent);
@@ -747,7 +750,7 @@ add_interface(
 	)
 {
 	/* Calculate the refid */
-	ep->addr_refid = addr2refid(&ep->sin);
+	ntp_be32enc(ep->addr_refid, addr2refid(&ep->sin));
 	/* link at tail so ntpq -c ifstats index increases each row */
 	LINK_TAIL_SLIST(ep_list, ep, elink, endpt);
 	ninterfaces++;


=====================================
ntpd/ntp_proto.c
=====================================
@@ -954,10 +954,10 @@ clock_update(
 	poll_update(peer, sys_poll);
 	sys_vars.sys_stratum = min(peer->stratum + 1, STRATUM_UNSPEC);
 	if (peer->stratum == STRATUM_REFCLOCK ||
-	    peer->stratum == STRATUM_UNSPEC)
-		sys_vars.sys_refid = peer->refid;
+		peer->stratum == STRATUM_UNSPEC)
+		memcpy(&sys_vars.sys_refid, &peer->refid, REFIDLEN);
 	else
-		sys_vars.sys_refid = addr2refid(&peer->srcadr);
+	    ntp_be32enc(sys_vars.sys_refid, addr2refid(&peer->srcadr));
 	/*
 	 * Root Dispersion (E) is defined (in RFC 5905) as:
 	 *
@@ -1537,14 +1537,16 @@ clock_select(void)
 		 * orphan mode in timer().
 		 */
 		if (peer->stratum == sys_orphan) {
+			char addrhash[REFIDLEN];
 			uint32_t	localmet;
 			uint32_t peermet;
 
 			if (peer->dstadr != NULL)
-				localmet = ntohl(peer->dstadr->addr_refid);
+				localmet = ntp_be32dec(peer->dstadr->addr_refid);
 			else
 				localmet = UINT32_MAX;
-			peermet = ntohl(addr2refid(&peer->srcadr));
+			ntp_be32enc(addrhash, addr2refid(&peer->srcadr));
+			peermet = ntp_be32dec(addrhash);
 			if (peermet < localmet && peermet < orphmet) {
 				typeorphan = peer;
 				orphmet = peermet;
@@ -2070,7 +2072,7 @@ peer_xmit(
 		xpkt.stratum = 0;
 		xpkt.ppoll = 0;
 		xpkt.precision = 0x20;
-		xpkt.refid = 0;
+		memset(&xpkt.refid, '\0', REFIDLEN);
 		xpkt.rootdelay = 0;
 		xpkt.rootdisp =	0;
 		xpkt.reftime = htonl_fp(0);
@@ -2084,7 +2086,7 @@ peer_xmit(
 		xpkt.stratum = STRATUM_TO_PKT(sys_vars.sys_stratum);
 		xpkt.ppoll = peer->hpoll;
 		xpkt.precision = sys_vars.sys_precision;
-		xpkt.refid = sys_vars.sys_refid;
+		memcpy(&xpkt.refid, &sys_vars.sys_refid, REFIDLEN);
 		xpkt.rootdelay = HTONS_FP(DTOUFP(sys_vars.sys_rootdelay));
 		xpkt.rootdisp =	 HTONS_FP(DTOUFP(sys_vars.sys_rootdisp));
 		xpkt.reftime = htonl_fp(sys_vars.sys_reftime);
@@ -2215,7 +2217,7 @@ fast_xmit(
 		xpkt.stratum = STRATUM_TO_PKT(sys_vars.sys_stratum);
 		xpkt.ppoll = max(rbufp->pkt.ppoll, ntp_minpoll);
 		xpkt.precision = sys_vars.sys_precision;
-		xpkt.refid = sys_vars.sys_refid;
+		memcpy(&xpkt.refid, &sys_vars.sys_refid, REFIDLEN);
 		xpkt.rootdelay = HTONS_FP(DTOUFP(sys_vars.sys_rootdelay));
 		xpkt.rootdisp = HTONS_FP(DTOUFP(sys_vars.sys_rootdisp));
 


=====================================
ntpd/ntp_refclock.c
=====================================
@@ -12,6 +12,7 @@
 #include "lib_strbuf.h"
 #include "ntp_calendar.h"
 #include "timespecops.h"
+#include "ntp_endian.h"
 
 #include <stdio.h>
 
@@ -217,7 +218,7 @@ refclock_newpeer(
 		refclock_unpeer(peer);
 		return false;
 	}
-	peer->refid = pp->refid;
+	memcpy(&peer->refid, &pp->refid, REFIDLEN);
 	return true;
 }
 
@@ -858,8 +859,10 @@ refclock_control(
 			pp->fudgetime2 = in->fudgetime2;
 		if (in->haveflags & CLK_HAVEVAL1)
 			peer->stratum = pp->stratum = (uint8_t)in->fudgeval1;
-		if (in->haveflags & CLK_HAVEVAL2)
-			peer->refid = pp->refid = in->fudgeval2;
+		if (in->haveflags & CLK_HAVEVAL2) {
+			ntp_be32enc(peer->refid,in->fudgeval2);
+			ntp_be32enc(pp->refid,in->fudgeval2);
+		}
 		if (in->haveflags & CLK_HAVEFLAG1) {
 			pp->sloppyclockflag &= ~CLK_FLAG1;
 			pp->sloppyclockflag |= in->flags & CLK_FLAG1;
@@ -883,7 +886,7 @@ refclock_control(
 	 */
 	if (out != NULL) {
 		out->fudgeval1 = pp->stratum;
-		out->fudgeval2 = pp->refid;
+		out->fudgeval2 = ntp_be16dec(pp->refid);
 		out->haveflags = CLK_HAVEVAL1 | CLK_HAVEVAL2;
 		out->fudgetime1 = pp->fudgetime1;
 		if (!D_ISZERO_NS(out->fudgetime1))


=====================================
ntpd/ntp_timer.c
=====================================
@@ -234,9 +234,13 @@ timer(void)
 			sys_vars.sys_leap = LEAP_NOWARNING;
 		}
 		sys_vars.sys_stratum = (uint8_t)sys_orphan;
-		if (sys_vars.sys_stratum > 1)
-			sys_vars.sys_refid = htonl(LOOPBACKADR);
-		else
+		if (sys_vars.sys_stratum > 1) {
+		    /* set LOOPBACKADR */
+		    sys_vars.sys_refid[0] = 127;
+		    sys_vars.sys_refid[1] = 0;
+		    sys_vars.sys_refid[2] = 0;
+		    sys_vars.sys_refid[3] = 1;
+		} else
 			memcpy(&sys_vars.sys_refid, "LOOP", REFIDLEN);
 		sys_offset = 0;
 		sys_vars.sys_rootdelay = 0;


=====================================
ntpd/refclock_generic.c
=====================================
@@ -24,6 +24,7 @@
 
 #include "config.h"
 #include "ntp_types.h"
+#include "ntp_endian.h"
 #include "timespecops.h"
 
 /*
@@ -2616,7 +2617,7 @@ parse_start(
 	if (peer->stratum <= 1)
 	    memmove((char *)&parse->generic->refid, parse->parse_type->cl_id, REFIDLEN);
 	else
-	    parse->generic->refid = htonl(PARSEHSREFID);
+	    ntp_be32enc(parse->generic->refid, PARSEHSREFID);
 
 	parse->generic->io.fd = fd232;
 


=====================================
ntpd/refclock_modem.c
=====================================
@@ -909,7 +909,7 @@ modem_timecode(
 	 * telephone networks the propatation time can be different for
 	 * each call and can reach 200 ms for some calls.
 	 */
-	peer->refid = pp->refid;
+	memcpy(&peer->refid, &pp->refid, REFIDLEN);
 	pp->lastrec = up->tstamp;
 	if (up->msgcnt == 0)
 		return;



View it on GitLab: https://gitlab.com/NTPsec/ntpsec/commit/ebeb660e6160965be6b36cd1090480376424312b

-- 
View it on GitLab: https://gitlab.com/NTPsec/ntpsec/commit/ebeb660e6160965be6b36cd1090480376424312b
You're receiving this email because of your account on gitlab.com.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.ntpsec.org/pipermail/vc/attachments/20190128/a12d6818/attachment-0001.html>


More information about the vc mailing list