SNTPv4 server based on rfc 4330 in C++

Question

Please review my SNTPv4 server based on rfc 4330

I tested running 3 instances of ntp-check.exe from Galleon systems and 1 instance of Microsoft w32tm - w32tm /stripchart /computer:

No crash!

This is a Linux implementation. The socket layer uses UDP and select.

Points of interest:

1. Use of a callback function to separate low level socket handling logic from protocol logic - the NTP protocol.
2. Address class as wrapper for socket API addresses.

Some specific concerns:

1. Is there a nicer way to pass the member function than using bind on line 133 in ntp-server.cpp
2. NTP-server code is fairly grungy, any ideas on how to improve.

Address.cpp:

#include "address.hpp"

#include <iostream>
#include <iomanip>

std::ostream& operator<<(std::ostream& os, const Address& address)
{
  os << "Address family : AF_INET (Internetwork IPv4)\n";
  os << "Port           : " << ntohs(address.sock_addr_.sin_port) << '\n';
  os << "IP address     : " << address.get_ip() << std::endl;
  return os;
}

Address::Address(const sockaddr_in& sock_address) : sock_addr_({ sock_address }) {}

// use * for INADDR_ANY
Address::Address(const char* dotted_decimal, const unsigned short port) : sock_addr_({}) {
  if (!dotted_decimal || strlen(dotted_decimal) == 0 || strcmp(dotted_decimal, "*") == 0) {
    sock_addr_.sin_addr.s_addr = INADDR_ANY;  // bind to all interfaces
  }
  else {
    if (inet_pton(AF_INET, dotted_decimal, &sock_addr_.sin_addr) != 1) {
      throw std::invalid_argument("invalid IPv4 address");
    }
  }

  sock_addr_.sin_family = PF_INET;
  sock_addr_.sin_port = htons(port);
}

Address::Address(const uint32_t ipv4_address, const unsigned short port) : sock_addr_({}) {
  sock_addr_.sin_family = PF_INET;
  sock_addr_.sin_addr.s_addr = ipv4_address;
  sock_addr_.sin_port = htons(port);
}

const sockaddr_in* Address::get() const {
  return &sock_addr_;
}

size_t Address::size() const {
  return sizeof sock_addr_;
}

std::string Address::get_ip() const {
  char buffer[64];
  const char* ipv4 = inet_ntop(PF_INET, &sock_addr_.sin_addr, buffer, 64);
  return ipv4 ? ipv4 : "";
}

Address.hpp:

/*
C++ wrapper for C socket API sockaddr_in
*/
#ifndef ADDRESS_HPP_
#define ADDRESS_HPP_

#include <string>
#include <cstring>
#include <cstdint>
#include <stdexcept>
#include <iostream>

#ifdef _WIN32
#include <winsock2.h>  // Windows sockets v2
#include <ws2tcpip.h> // WinSock2 Extension, eg inet_pton, inet_ntop, sockaddr_in6
#elif __linux__ || __unix__
#include <arpa/inet.h>
#else
#error Unsupported platform
#endif

class Address {
public:
  //! construct from sockaddr_in
  Address(const sockaddr_in& sock_address);
  //! construct from 32 bit unsigned integer and a port
  Address(const uint32_t ipv4_address, const unsigned short port);
  //! construct from IP address string and port, use * for INADDR_ANY
  Address(const char* dotted_decimal, const unsigned short port);
  //! retrieve socket API sockaddr_in
  const sockaddr_in* get() const;
  //! get size of sockaddr_in
  size_t size() const;
  //! get IP address as string
  std::string get_ip() const;

  friend std::ostream& operator<<(std::ostream& os, const Address& address);

private:
  sockaddr_in sock_addr_;
};

//! debug print address
std::ostream& operator<<(std::ostream& os, const Address& address);

#endif // ADDRESS_HPP_

main.cpp:

#include <iostream>

#include "ntp-server.hpp"

int main(int argc, char* argv[]) {

  std::cout << "Starting SNTPv4 Server\n";

  ntp_server server(123);
  server.run();
}

Makefile:

CXX=g++
CXXFLAGS=-Wall -std=c++17
PROJECTNAME=ntpserver

SOURCES = main.cpp select-server.cpp ntp-server.cpp address.cpp

OBJ = $(SOURCES:.cpp=.o)

# $@ is target
# $^ is pre-requisites - ie $(OBJ)
$(PROJECTNAME): $(OBJ)
    $(CXX) $(LDFLAGS) $^ $(LDLIBS) -o $@

debug: CPPFLAGS += -DDEBUG
debug: CXXFLAGS += -g
debug: $(PROJECTNAME)

# $< is name of first pre-requisite
%.o: %.c
    $(CXX) $(CPPFLAGS) $(CXXFLAGS) -c $<

clean:
    $(RM) -rf *.o $(PROJECTNAME)

ntp-server.cpp:

#include "ntp-server.hpp"

#include <stdio.h>
#include <time.h> /* for time() and ctime() */
#include <sys/time.h> // gettimeofday

namespace {

/* unix epoch is 1970-01-01 00:00:00 +0000 (UTC) but start of ntp time
   is 1900-01-01 00:00:00 UTC, so adjust with difference */
const uint32_t NTP_UTIME_DIFF = 2208988800U; /* 1970 - 1900 */
const uint64_t NTP_SCALE_FRAC = 4294967296;

// utility function to print an array in hex
void printhex (const void *buf, size_t len)
{
  for (size_t i = 0; i < len; i++) {
    printf ("%02X", ((uint8_t *)buf)[i]);
  }
  fputc ('\n', stdout);
}

/* get timestamp for NTP in LOCAL ENDIAN, in/out arg is a uint32_t[2] array with
   most significant
   32 bit part no. seconds since 1900-01-01 00:00:00 and least significant 32 
   bit part fractional seconds */
void gettime64(uint32_t ts[])
{
  struct timeval tv;
  gettimeofday(&tv, NULL);

  ts[0] = tv.tv_sec + NTP_UTIME_DIFF;
  ts[1] = (NTP_SCALE_FRAC * tv.tv_usec) / 1000000UL;
}

/* get timestamp for NTP in LOCAL ENDIAN, returns uint64_t with most significant
   32 bit part no. seconds since 1900-01-01 00:00:00 and least significant 32 
   bit part fractional seconds */
uint64_t gettime64()
{
  struct timeval tv_unix;
  gettimeofday(&tv_unix, NULL);

  uint64_t ntp_secs = tv_unix.tv_sec + NTP_UTIME_DIFF;
  uint64_t ntp_usecs = (NTP_SCALE_FRAC * tv_unix.tv_usec) / 1000000UL;

  return (ntp_secs << 32) | ntp_usecs;
}

// helper to populate an array at start_index with uint32_t  
void populate_32bit_value(unsigned char* buffer, int start_index, uint32_t value) {
  uint32_t* p32 = reinterpret_cast<uint32_t*>(&buffer[start_index]);
  *p32 = value;
}

/* create the NTP response message to be sent to client 
   Args:
   recv_buf - array received from NTP client (should be 48 bytes in length)
   recv_time - array containing time NTP request received
   send_buf - byte array to be sent to client
*/
void make_reply(const unsigned char recv_buf[], uint32_t recv_time[], unsigned char* send_buf) {

  /* LI VN Mode
     Leap Indicator = 0
     Version Number = 4 (SNTPv4)
     Mode = 4 = server
     0x24 == LI=0, version=4 (SNTPv4), mode=4 (server)  00 100 100 */
  send_buf[0] = 0x24;

  /* Stratum = 1 (primary reference). A stratum 1 level NTP server is
     synchronised by a reference clock, eg in UK the Anthorn Radio Station
     in Cumbria. (Not true - next project work out how to sync up with radio
     signal. Typically in a real world scenario, subsidiary ntp servers at
     lower levels of stratum would sync with a stratum ntp server. */
  send_buf[1] = 0x1;

  // Poll Interval - - we set to max allowable poll interval
  send_buf[2] = 0x11;  // 17 == 2^17 (exponent)

  // Precision
  send_buf[3] = 0xFA;  // 0xFA == -6 - 2^(-6) == mains clock frequency

  // *** below are 32 bit values

  /* Root Delay - total roundtrip delay to primary ref source in seconds
     set to zero - simplification */
  populate_32bit_value(send_buf, 4, 0);

  /* Root Dispersion - max error due to clock freq tolerance in secs, svr sets
     set to zero (simplification) */
  populate_32bit_value(send_buf, 8, 0);


  /* Reference Identifier - reference source, LOCL means uncalibrated local clock
     We must send in network byte order (we assume we built svr little endian) */ 
  uint32_t refid = htonl(('L' << 24) | ('O' << 16) | ('C' << 8) | 'L');
  populate_32bit_value(send_buf, 12, refid);

  // *** below are 64 bit values

  /* Reference Timestamp - time system clock was last set or corrected
     investigate - if we assume client is requesting every poll interval 2^17 - just simulate what time was back then
     2^17 = 131072 */
  uint64_t ntp_now = gettime64();

  uint32_t p32_seconds_before = htonl((ntp_now >> 32) - 131072);
  uint32_t p32_frac_seconds_before = htonl(ntp_now & 0xFFFFFFFF);

  populate_32bit_value(send_buf, 16, p32_seconds_before);
  populate_32bit_value(send_buf, 20, p32_frac_seconds_before);
  
  /* Originate Timestamp: This is the time at which the request departed
     the client for the server, in 64-bit timestamp format.  We can copy
     value from client request */
  memcpy(&send_buf[24], &recv_buf[40], 4);
  memcpy(&send_buf[28], &recv_buf[44], 4);

  // Receive Timestamp - get from time rq received by server
  uint32_t* p32 = reinterpret_cast<uint32_t*>(&send_buf[32]);
  *p32++ = htonl(recv_time[0]);  // seconds part
  *p32++ = htonl(recv_time[1]);  // fraction of seconds part

  // Transmit Timestamp - re-use ntp_now time obtained above
  populate_32bit_value(send_buf, 40, htonl(ntp_now >> 32));
  populate_32bit_value(send_buf, 44, htonl(ntp_now & 0xFFFFFFFF));  
}

}  // unnamed namespace

// is there any way to do this and not have to use bind.  It is a little ugly
ntp_server::ntp_server(uint16_t port)
  : udp_server_(port, std::bind(&ntp_server::read_callback, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3)) { }

  void ntp_server::run() {
    udp_server_.run();
}

void ntp_server::read_callback(const char* data, const size_t length, const Address& address) {
  const size_t ntp_msg_size{48};

  std::cout << "new data in\n";
  printhex(data, length);

  std::cout << "Client address:\n" << address << std::endl;

  uint32_t recv_time[2];
  gettime64(recv_time);
  
  unsigned char send_buf[ntp_msg_size] {};

  make_reply(reinterpret_cast<const unsigned char*>(data), recv_time, send_buf);

  std::cout << "data to send:\n";
  printhex(send_buf, ntp_msg_size);

  ssize_t ret;
  if ( (ret = udp_server_.send(reinterpret_cast<const char*>(send_buf), ntp_msg_size, address)) != ntp_msg_size) {
    std::cerr << "Error sending response to client: " << ret;
    perror("sendto");
  }
}

ntp-server.hpp:

/*
Basic implementation of v4 SNTP server as per: https://www.rfc-editor.org/rfc/rfc4330

Uses udp_server for low level UDP socket communication

Separation of socket and ntp handling via passing a callback function to udp server
*/
#ifndef NTP_SERVER_HPP_
#define NTP_SERVER_HPP_

#include "select-server.hpp"

class ntp_server {
public:
  //! initialise ntp_server with server port, defaults to well known NTP port 123
  ntp_server(uint16_t port = 123);

  //! start NTP server
  void run();

  //! callback to handle data received from NTP client
  void read_callback(const char* data, const size_t length, const Address& address);

private:
  udp_server udp_server_;
};

#endif // NTP_SERVER_HPP_

select-server.cpp:

#include "select-server.hpp"

#include <sys/socket.h>
#include <netinet/in.h>
#include <errno.h>
#include <string.h>
#include <unistd.h>
#include <arpa/inet.h>

#include <iostream>

udp_server::udp_server(uint16_t port, client_request_callback request_callback) : port_(port), rq_callback_(request_callback ), s_(0) {

  std::cout << "udp_server will bind to port: " << port_ << std::endl;
}

udp_server::~udp_server() {
  if (s_) {
    close(s_);
  }
}

//! start server
void udp_server::run() {

  // create server socket
  s_ = socket(PF_INET, SOCK_DGRAM, 0);
  std::cout << "Socket created: " << s_ << "\n";

  // bind the server address to the socket
  sockaddr_in server_addr = {}; // AF_INET
  server_addr.sin_family = AF_INET;
  server_addr.sin_port = htons(port_);
  server_addr.sin_addr.s_addr = INADDR_ANY;
  socklen_t len_inet = sizeof server_addr;
  int r = bind(s_, reinterpret_cast<sockaddr*>(&server_addr), len_inet);

  if ( r == -1) {
    std::cerr << "bind returned: " << r << strerror(errno) << std::endl;
    exit(1);
  }

  // express interest in socket s for read events
  fd_set rx_set; // read set
  FD_ZERO(&rx_set); // init
  int maxfds = s_ + 1;

  // start the server loop
  for (;;) {
    FD_SET(s_, &rx_set);

    // sample timeout of 2.03 secs
    timeval tv; // timeout value
    tv.tv_sec = 2;
    tv.tv_usec = 30000;

    int n = select(maxfds, &rx_set, NULL, NULL, &tv);

    if ( n == -1) {
      std::cerr << "select returned: " << n << strerror(errno) << std::endl;
      exit(1);
    } else if ( !n ) {
      // select timeout
      continue;
    }

    // if udp socket is readable receive the message.
    if (FD_ISSET(s_, &rx_set)) {
      sockaddr_in sock_address {};
      unsigned char buf[48] {}; // I/O buffer
      socklen_t len_client = sizeof sock_address;

      // retrieve data received
      ssize_t recbytes = recvfrom(s_, buf, sizeof(buf), 0,
                          reinterpret_cast<sockaddr*>(&sock_address), &len_client);

      if (recbytes <= 0) {
    std::cerr << "recvfrom returned: " << recbytes << std::endl;
        continue;
      }
      
      // Create an address from client_address and pass to callback function with data
      Address client_address(sock_address);
      rq_callback_(reinterpret_cast<const char*>(buf), recbytes, client_address);
      
      FD_CLR(s_, &rx_set);
    }
  } // for loop
}

//!  returns number of bytes successfully sent
ssize_t udp_server::send(const char* data, const size_t length, const Address& address) {
  return sendto(s_, data, length, 0, (const sockaddr*)address.get(), address.size());
}

select-server.hpp:

/*
Basic implementation of UDP socket server using select

User provides callback function to handle client requests
*/
#ifndef SELECT_SERVER__
#define SELECT_SERVER__

#include <functional>

#include <cstdint>

#include "address.hpp"

// callback signature for user provided function for handling request data from clients
using client_request_callback = std::function<void(const char*, const size_t, const Address&)>;


class udp_server {
public:
  //! construct with port and callback for custom data handler
  udp_server(uint16_t port, client_request_callback request_callback);

  //! destructor
  virtual ~udp_server();

  //! call run to start server
  void run();
  
  //!  send returns number of bytes successfully sent
  ssize_t send(const char* data, const size_t length, const Address& address);
  
private:
  uint16_t port_;
  client_request_callback rq_callback_;
  int s_;
};
  


#endif // SELECT_SERVER__

Answer 1

General

It's better to present the files in a more logical order - headers before implementation files, and lower-level utilities before the higher-level code that depends on them.

Address.cpp

The operator<<() shouldn't be flushing os - that's really something that should be decided by the caller rather than imposed at this level.

The constructors look odd with ({…}) where plain {…} would be sufficient for the initializers.

There's no declaration for strlen or strcmp - I think we should be including <cstring> and using std::strlen and std::strcmp here. Or better, ditch the C-style string handling and use a std::string_view instead.

Address.hpp

This header has unnecessary includes for <cstring> and <stdexcept>, and can be slimmed greatly by using <iosfwd> instead of <iostream>.

std::size_t is misspelt (this identifier lives in the std namespace).

The one-argument constructor should be explicit.

The get() function is lacking any indication of the ownership and lifetime of the returned pointer - prefer not to return pointers unless it's unavoidable. The name is a bit vague, too.

main.cpp

We could simplify the makefile if we called this ntpserver.cpp instead (so we could use the default %.o rule).

argc and argv are unused - perhaps we should be using the int main() signature instead.

It probably makes more sense to use std::clog for the status message than std::cout.

Makefile

I recommend CFLAGS += -Wextra at least, and usually some more diagnostic options (-Wconversion highlights a few places in this program). That will greatly improve code quality (assuming you act on the warnings, of course).

SOURCES appears to be used only to define OBJECTS, so perhaps omit that and define OBJECTS only.

The comments in this file are useless. They just tell us what every makefile author already knows, and nothing about why the Makefile is written as it is.

The debug target is problematic. When invoked, it will rebuild the modified sources using different compiler flags, likely resulting in a binary made with a mix of debug and non-debug flags. Much better to have a separate build with debug (we usually do that by building in a separate directory from the same sources, using VPATH).

%.o: %.c target is pointless, since it effectively duplicates the built-in rule for this.

We're completely missing the dependencies that object files have on the headers. I recommend getting g++ to generate dependency files by supplying one of the -M options.

clean target doesn't need -r as none of its arguments should be a directory. And it doesn't need -f because that's included in the definition of $(RM).

.PHONY and .DELETE_ON_ERROR targets are missing.

ntp-server.cpp

Why are we including the deprecated C header <stdio.h> instead of the C++ version <cstdio>?

The comment for including <time.h> says we need it for time() and ctime(), but we never use those - either drop it, or change to <ctime> and fix the comment.

No definition of uint8_t, uint32_t and uint64_t - presumably we intended to include <cstdint> and use the corresponding names from std.

printhex() does more than its comment says, as it writes an additional newline after printing the hex content of the array.

The two implementations of gettime64() have quite a lot of duplicated code. It would be better for the second to simply call the first:

uint64_t gettime64()
{
    uint32_t ts[2];
    gettime64(ts);
    return (ts[0] << 32) + ts[1];
}

In make_reply, it's unclear why we write the root delay and root dispersion in native byte-order rather than network order - that definitely needs an explanatory comment.

refid has an inbuilt assumption that we're building for an ASCII (or compatible) platform - requires either a comment or a code change to eliminate that assumption, e.g. refid = 0x4c4f434c.

We need to include cstring to get a declaration of std::memcpy. And we can combine the two calls into one with twice the length.

read_callback produces a lot to std::cout that ought to be going to the log stream.

ntp-server.hpp

Many missing includes - don't rely on "select-server.hpp" bringing in everything we need, as that makes it fragile.

Again, attempting to use uint16_t and size_t from the global namespace instead of std.

Constructor should be explicit.

select-server.cpp

Again, prefer C++ standard headers <cerrno> and <cstring>.

The destructor avoids closing the file descriptor when it is 0, but the failure return from socket() is -1, so that test is incorrect.

The constructor should return early when socket() fails - bind() isn't going to succeed either. It's probably best to use std::perror() for both of these.

Include <cstdlib> to declare std::exit().

select() is unnecessary when we're listening to a single file descriptor. But perhaps we should be listening to more - standard practice is to use a pipe so that a signal handler can tell the loop to exit cleanly.

The fixed timeout is also pointless, given that all we do is repeat the select() if it expires.

The hardcoded receive buffer size (48) could be too small for most applications - this looks to have been determined by SNTP, and it makes this class much less reusable than I'd like.

select-server.hpp

The familiar misspellings of std::size_t and std::uint16_t.

Instead of providing callback as a std::function, consider requiring implementers to subclass and implement a pure-static function. That's why we provided a virtual destructor, isn't it?