OO simple network time server with changes from a previous code inspection and added Windows support

Question

I put up my SNTP Server code for review here: SNTPv4 server based on rfc 4330 in C++

a fortnight ago and since then I have made changes as per code review comments and also added support for Windows, mostly just by adding the winsock startup and shutdown functions and I also made the Makefile work for Windows in addition to Linux.

For the Makefile to work on Windows you need to install GNU make for Windows and assuming you already have Visual Studio installed, run the Native Tools Command Prompt for Visual Studio to setup the required env variables. I only tested with 64 bit building on Windows.

Is creating a Makefile to work under Windows a fools errand?

Is the code clear?

How could it be improved?

Very interested to hear any feedback on the code.

Address.hpp:

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

#include <string>
#include <cstdint>
#include <iosfwd>

#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
  explicit Address(const sockaddr_in& sock_address);
  //! construct from 32 bit unsigned integer and a port
  Address(const std::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
  std::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_

Address.cpp:

#include "address.hpp"

#include <iostream>
#include <iomanip>
#include <cstring>
#include <stdexcept>


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() << '\n';
  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 || std::strlen(dotted_decimal) == 0 || std::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 : "";
}

udp_server.hpp:

/*
Basic implementation of UDP socket server using select

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

#ifdef _WIN32
#ifdef _WIN64
typedef __int64 ssize_t;
#else
typedef int     ssize_t;
#endif
#endif

#include <functional>
#include <vector>
#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, callback for custom data handler and receive buffer size
  udp_server(uint16_t port, client_request_callback request_callback, size_t buffer_size = 4096);

  //! 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_;
  //const size_t buffer_size_;
  std::vector<char> buffer_;
  int s_;
};

#endif // UDP_SERVER__

udp_server.cpp:

#ifdef _WIN32
#include <winsock2.h>  // Windows sockets v2
#include <ws2tcpip.h> // WinSock2 Extension, eg inet_pton, inet_ntop, sockaddr_in6

namespace {
    int close(int fd) {
        return closesocket(fd);
    }
}

#ifdef _WIN64
typedef __int64 ssize_t;
#else
typedef int     ssize_t;
#endif

#elif __linux__ || __unix__
#include <sys/socket.h>
#include <netinet/in.h>
#include <unistd.h>
#include <arpa/inet.h>
#else
#error Unsupported platform
#endif

#include <iostream>
#include <cerrno>
#include <cstring>
#include <cstdint>
#include <cstdlib> // std::exit()

#include "udp-server.hpp"


namespace {
  void bailout(const char* msg) {
    std::perror(msg);
    std::exit(1);
  }
}

udp_server::udp_server(uint16_t port, client_request_callback request_callback, size_t buffer_size) : port_(port), rq_callback_(request_callback ), buffer_(buffer_size), s_(-1) {

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

udp_server::~udp_server() {
  if (s_ != -1) {
    close(s_);
  }
#ifdef _WIN32
  WSACleanup();
#endif
}

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

#ifdef _WIN32
    WSADATA w = { 0 };
    int error = WSAStartup(0x0202, &w);

    if (error || w.wVersion != 0x0202)
    { // there was an error
        throw "Could not initialise Winsock2\n";
    }
#endif

  // create server socket
  s_ = socket(PF_INET, SOCK_DGRAM, 0);
  if (s_ == -1) {
    bailout("socket error");
  } 

  // 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) {
    bailout("bind error");
  }

  // 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);

    // block until socket activity
    int n = select(maxfds, &rx_set, NULL, NULL, NULL);
    if (n == -1) {
      bailout("select error");
    } else if ( !n ) {
      // select timeout
      continue;
    }

    // if udp socket is readable receive the message.
    if (FD_ISSET(s_, &rx_set)) {
      sockaddr_in sock_address {};
      socklen_t len_client = sizeof sock_address;

      // retrieve data received
      ssize_t recbytes = recvfrom(s_, buffer_.data(), buffer_.size(), 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_(buffer_.data(), 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(), static_cast<int>(address.size()));
}

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 "udp-server.hpp"

#include <cstdint>

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

  //! start NTP server
  void run();

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

private:
  udp_server udp_server_;
};

#endif // NTP_SERVER_HPP_

ntp-server.cpp:

#include "ntp-server.hpp"

#include <cstdio>
#include <cstring> // memcpy
#include <iostream>
#include <iomanip>

#ifdef _WIN32
int gettimeofday(struct timeval* tp, struct timezone* tzp)
{
    // Note: some broken versions only have 8 trailing zero's, the correct epoch has 9 trailing zero's
    // This magic number is the number of 100 nanosecond intervals since January 1, 1601 (UTC)
    // until 00:00:00 January 1, 1970 
    static const std::uint64_t EPOCH = ((std::uint64_t)116444736000000000ULL);

    SYSTEMTIME  system_time;
    FILETIME    file_time;
    std::uint64_t time;

    GetSystemTime(&system_time);
    SystemTimeToFileTime(&system_time, &file_time);
    time = ((std::uint64_t)file_time.dwLowDateTime);
    time += ((std::uint64_t)file_time.dwHighDateTime) << 32;

    tp->tv_sec = (long)((time - EPOCH) / 10000000L);
    tp->tv_usec = (long)(system_time.wMilliseconds * 1000);
    return 0;
}
#else
#include <sys/time.h> // gettimeofday
#endif

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 std::uint32_t NTP_UTIME_DIFF = 2208988800U; /* 1970 - 1900 */
const std::uint64_t NTP_SCALE_FRAC = 4294967296;
const std::size_t ntp_msg_size{48};

// utility function to print an array in hex
template<typename T>  
void printhex (T* buf, std::size_t len)
{
  auto previous_flags = std::clog.flags();
  for (std::size_t i = 0; i < len; i++) {
    std::clog << std::hex << std::setfill('0') << std::setw(2) << (buf[i] & 0xFF) << ' ';
  }
  std::clog.setf(previous_flags);
}

/* 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(std::uint32_t ts[])
{
  struct timeval tv;
  gettimeofday(&tv, NULL);
  if (tv.tv_sec > 0xFFFFFFFF) {
    std::clog << "timeval time_t seconds value > 0xFFFFFFFF, (" << tv.tv_sec << ") and will overflow\n";
  }
  ts[0] = static_cast<uint32_t>(tv.tv_sec) + NTP_UTIME_DIFF;  // ntp seconds

  if ((NTP_SCALE_FRAC * tv.tv_usec) / 1000000UL > 0xFFFFFFFF) {
    std::clog << "timeval fractional seconds value > 0xFFFFFFFF, (" << tv.tv_sec << ") and will overflow\n";
  }
  ts[1] = static_cast<uint32_t>((NTP_SCALE_FRAC * tv.tv_usec) / 1000000UL);  //ntp usecs
}

/* 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()
{
  uint32_t ts[2];
  gettime64(ts);
  uint64_t tmp = ts[0];
  return (tmp << 32) + ts[1];
}

// helper to populate an array at start_index with uint32_t  
void populate_32bit_value(unsigned char* buffer, int start_index, std::uint32_t value) {
  std::uint32_t* p32 = reinterpret_cast<std::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[], std::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, htonl(0));

  /* Root Dispersion - max error due to clock freq tolerance in secs, svr sets
     set to zero (simplification) */
  populate_32bit_value(send_buf, 8, htonl(0));
  
  /* Reference Identifier - reference source, LOCL means uncalibrated local clock
     We must send in network byte order (we assume we built svr little endian) */ 
  std::uint32_t refid ( 0x4c4f434c ); // LOCL in ASCII
  populate_32bit_value(send_buf, 12, htonl(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 */
  std::uint64_t ntp_now = gettime64();
  std::uint32_t p32_seconds_before = htonl(static_cast<uint32_t>(ntp_now >> 32) - 131072);
  std::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 */
  std::memcpy(&send_buf[24], &recv_buf[40], 8);

  // Receive Timestamp - get from time rq received by server
  std::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(static_cast<uint32_t>(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(std::uint16_t port)
  : udp_server_(port,
        std::bind(&ntp_server::read_callback, this, std::placeholders::_1, std::placeholders::_2, std::placeholders::_3),
        ntp_msg_size) { }

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

void ntp_server::read_callback(const char* data, const std::size_t length, const Address& address) {

  std::uint32_t recv_time[2];
  gettime64(recv_time);
  
  std::clog << "new data in\n";
  printhex(data, length);
  std::clog << '\n';

  std::clog << "Client address:\n" << address << std::endl;
  
  unsigned char send_buf[ntp_msg_size] {};

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

  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");
  }

  std::clog << "data sent:\n";
  printhex(send_buf, ntp_msg_size);
  std::clog << std::endl;
}

main.cpp:

#include <iostream>

#include "ntp-server.hpp"

int main() {

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

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

Makefile:

ifeq ($(OS),Windows_NT)
    UNAME := Windows
    CXX=cl.exe
    CXXFLAGS=-EHs -nologo -std:c++17
    RM=rd /s /q
    MKDIR_P=mkdir
    LINKER=link.exe
    LIBS=ws2_32.lib
    LFLAGS=-MACHINE:X64 -nologo -SUBSYSTEM:CONSOLE
    OBJSFX=.obj
    OUTCC=-Fo$@
    OUTLINK=-out:$@
    DBGCFLAGS = -DDEBUG -W4 -MTd -TP -Od
    RELCFLAGS = -DNDEBUG -W4 -MT -TP -O2
    EXE = ntpserver.exe
else
    UNAME := $(shell uname -s)
    CXX=g++
    CXXFLAGS=-Wall -std=c++17 -Wextra -Wconversion
    MKDIR_P=mkdir -p
    LINKER=g++
    RM=rm -rf
    OBJSFX=.o
    OUTCC=-o $@
    OUTLINK=-o $@
    DBGCFLAGS = -DDEBUG -g
    RELCFLAGS = -DNDEBUG
    EXE = ntpserver
endif

SOURCES = main.cpp udp-server.cpp ntp-server.cpp address.cpp
OBJS = $(SOURCES:.cpp=$(OBJSFX))

# Debug build settings
DBGDIR = debug
DBGEXE = $(DBGDIR)/$(EXE)
DBGOBJS = $(addprefix $(DBGDIR)/, $(OBJS))

# Release build settings
RELDIR = release
RELEXE = $(RELDIR)/$(EXE)
RELOBJS = $(addprefix $(RELDIR)/, $(OBJS))

.PHONY: all clean debug prep release remake

# Default build
all: prep release

# Debug rules
debug: $(DBGEXE)

$(DBGEXE): $(DBGOBJS)
    $(LINKER) $(LFLAGS) $^ $(LIBS) $(OUTLINK)

$(DBGDIR)/%$(OBJSFX): %.cpp
    $(CXX) -c $(CXXFLAGS) $(DBGCFLAGS) $< $(OUTCC)

# Release rules
release: $(RELEXE)

$(RELEXE): $(RELOBJS)
    $(LINKER) $(LFLAGS)  $^ $(LIBS) $(OUTLINK)

$(RELDIR)/%$(OBJSFX): %.cpp
    $(CXX) -c $(CXXFLAGS) $(RELCFLAGS) $< $(OUTCC)

# Other rules
prep:
    @$(MKDIR_P) $(DBGDIR) $(RELDIR)

remake: clean all

clean:
    $(RM) debug release

Answer 1

Answers to your questions

Is creating a Makefile to work under Windows a fools errand?

No, it can work fine under Windows. However, installing GNU make on Windows might be an annoying step for some. Also, your Makefile still makes assumptions about what compilers are available on what platforms. What if you want to use Clang on Linux, or GCC on Windows? What if you want to cross-compile a Windows binary on Linux?

Consider using a modern build system like Meson or CMake. These abstract away the low-level platform details. They are also a bit easier to install for Windows users.

Is the code clear?

It doesn't look very nice. Maybe that is a consequence of it being networking code and dealing with low-level protocol bits. See below for ways to improve things.

Documentation

I see you are writing Doyxgen-like comments in the header files. However, they are quite terse and are missing a lot of detail. Make sure you document every class member, the class itself, and for functions you should document all the parameters and the return value as well.

Consider running Doxygen on your code, and enabling warnings and errors, and fixing the ones it reports.

Don't declare friends twice

I see that you declared operator<<() for Address twice, once as a friend inside class Address, once outside. You should only need the friend declaration.

Naming things

Make sure names of classes, functions and variables are precise; they should convey what they represent accurately. Ideally, we shouldn't need to read the documentation to know what they are doing.

For example, in Address there is get() and get_ip(). Both actually return an IP address. From the name however it is completely unclear that get_ip() returns a human-readable string.

In udp_server, there is a member variable s_. What does this do? It's not even documented so I have to find where it is used and read the code to determine what it is.

There is also the issue of consistency: why is Address spelled with a capital, but udp_server and ntp_server are in lower case? Why is there sock_address but sock_addr_? Avoid unnecessary abbreviations, but if you do abbreviate, always do it in the same way.

Here is a list of recommended name change:

• get() -> sockaddr() (also see below)
• size() -> socklen()
• get_ip() -> to_string()
• sock_addr_ -> socket_address_
• rq_callback_ -> request_callback_
• s_ -> socket_fd_
• r -> result
• n -> number_fds_set, n_fds_set, or just reuse result.
• recvbytes -> received_bytes or n_received

Return appropriate types

You have get() and size() member functions for Address, but the main use is in passing the result to socket functions that expect a struct sockaddr* and socklen_t. So it might be better to return the latter types instead. The names of those functions could be changed to reflect that. Then the code using it becomes much cleaner:

ssize_t udp_server::send(const char* data, const size_t length, const Address& address) {
    return sendto(socket_fd_, data, length, 0, address.sockaddr(), address.socklen());
}

Don't make the destructor of udp_server virtual

Since nothing is inheriting from udp_server, there is no need to make its destructor virtual.

Consider using getaddrinfo() and getnameinfo()

The more modern way of converting address to text and back is to use getaddrinfo() and getnameinfo(). These functions are much more capable than inet_pton() and inet_ntop(), and as an added benefit they support IPv6 as well. I strongly recommend you move over to them.

Use C++'s time functions

Since you are already using POSIX and BSD network functions it is easy to start using other C functions as well. However, prefer using standard C++ functions where possible. For time, there is std::chrono. The equivalent of gettimeofday() is std::chrono::system_clock::now(). (C++20 introduced more clocks, maybe one of them is even better suited for NTP.) As a benefit, the C++ functions are cross-platform, so there is no need to reimplement gettimeofday() on Windows.

Building a message

make_reply() doesn't look nice at all. There are direct array accesses, calls to populate_32bit_value() and std::memcpy(), and writes via pointers. You have hardcoded offsets in several places. There is also the issue of byte swapping 64-bit values that doesn't have a nice function. Basically, this is a mess! There are two alternatives I would recommend:

Use a struct

An NTP replay has a well-defined format. Create a struct that represents this format. For example:

struct ntp_reply {
    std::uint8_t li_vn_mode;
    std::uint8_t stratum;
    std::uint8_t poll_interval;
    std::uint8_t precision;
    uint32_t root_delay;
    …
    uint64_t transmit_timestamp;
};

That way you can fill in the struct in a very clean way:

ntp_reply make_reply(…) {
    ntp_reply reply;

    reply.li_vn_mode = 0x24;
    reply.stratum = 1;
    …
    reply.transmit_timestamp = hton64(ntp_now);

    return reply;
}

Build a buffer in a consistent way

The other option is to build a buffer in a consistent way. For example, you could use a std::vector<std::uint8_t> to be the buffer, and have a function with overloads for the various types to append to this buffer:

template<typename T>
append(std::vector<sdt::uint8_t>& buffer, T value) {
    auto offset = buffer.size();
    buffer.resize(offset + sizeof value);
    std::memcpy(&buffer[offset], &value, sizeof value);
}

std::vector<std::uint8_t> make_reply(…) {
    std::vector<std::uint8_t> reply;

    append(reply, std::uint8_t(0x24));
    append(reply, std::uint8_t(1));
    …
    append(reply, hton64(ntp_now));

    return reply;
}

Create a function to byteswap 64-bit values

Create your own hton64() to change the endianness of 64-bit values. This cleans up to code that needs to call it significantly, and avoids potential mistakes.

Use std::uint64_t for 64-bit values

Instead of storing 64-bit values in an array of 32-bit integers, just use std::uint64_t.

Avoiding std::bind()

// is there any way to do this and not have to use bind. It is a little ugly

You could consider using a lambda expressions instead:

ntp_server::ntp_server(std::uint16_t port)
  : udp_server_(port,
        [this](auto&&... args){ read_callback(std::forward<decltype(args)>(args)...); },
        ntp_msg_size) { }

Slighly more compact, but using std::forward and decltype to make sure the arguments are passed correctly is a little ugly as well. Note that you can make the std::bind() call shorter by writing:

using std::placeholders;

ntp_server::ntp_server(std::uint16_t port)
  : udp_server_(port,
        std::bind(&ntp_server::read_callback, this, _1, _2, _3),
        ntp_msg_size) { }