NTP client in infinite loop (Not working )

Question

I have designed a client from the reference given here.

I modified the code (see below) to fulfill these requirements

1. print timestamps in milliseonds

2. within infinite loop for getting NTP timestamps, RTT etc.

3. to define a cycle time (get time every x second)

---

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
#include <netdb.h>
#include <time.h>

#define NTP_TIMESTAMP_DELTA 2208988800ull
#define ENDIAN_SWAP32(data)                                                    \
  ((data >> 24) |               /* right shift 3 bytes */                      \
   ((data & 0x00ff0000) >> 8) | /* right shift 1 byte */                       \
   ((data & 0x0000ff00) << 8) | /* left shift 1 byte */                        \
   ((data & 0x000000ff) << 24)) /* left shift 3 bytes */

void error(char *msg) {
  perror(msg); // Print the error message to stderr.
  exit(0);     // Quit the process.
}

int main(int argc, char *argv[]) {
  int sockfd, n;    // Socket file descriptor and the n return result from
                    // writing/reading from the socket.
  int portno = 123; // NTP UDP port number
  int i, z = 0;
  struct timeval tv1, tv2;

  typedef struct {
    unsigned li : 2; // Only two bits. Leap indicator.
    unsigned vn : 3; // Only three bits. Version number of the protocol.
    unsigned
        mode : 3; // Only three bits. Mode. Client will pick mode 3 for client.

    uint8_t stratum; // Eight bits. Stratum level of the local clock.
    uint8_t poll; // Eight bits. Maximum interval between successive messages.
    uint8_t precision; // Eight bits. Precision of the local clock.

    uint32_t rootDelay; // 32 bits. Total round trip delay time.
    uint32_t
        rootDispersion; // 32 bits. Max error aloud from primary clock source.
    uint32_t refId;     // 32 bits. Reference clock identifier.

    uint32_t refTm_s; // 32 bits. Reference time-stamp seconds.
    uint32_t refTm_f; // 32 bits. Reference time-stamp fraction of a second.

    uint32_t origTm_s; // 32 bits. Originate time-stamp seconds.
    uint32_t origTm_f; // 32 bits. Originate time-stamp fraction of a second.

    uint32_t rxTm_s; // 32 bits. Received time-stamp seconds.
    uint32_t rxTm_f; // 32 bits. Received time-stamp fraction of a second.

    uint32_t txTm_s; // 32 bits and the most important field the client cares
                     // about. Transmit time-stamp seconds.
    uint32_t txTm_f; // 32 bits. Transmit time-stamp fraction of a second.

  } ntp_packet; // Total: 384 bits or 48 bytes.

  // Create and zero out the packet. All 48 bytes worth.

  ntp_packet packet = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
  memset(&packet, 0, sizeof(ntp_packet));
  *((char *)&packet + 0) =
      0x1b; // Represents 27 in base 10 or 00011011 in base 2.

  uint8_t *ptr = (uint8_t *)(&packet); /* to read raw bytes */
  struct sockaddr_in serv_addr;        // Server address data structure.
  struct hostent *server;              // Server data structure.

  sockfd = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP); // Create a UDP socket.

  if (sockfd < 0)
    error("ERROR opening socket");

  server = gethostbyname(argv[1]); // Convert URL to IP.

  if (server == NULL)
    error("ERROR, no such host");
  // Zero out the server address structure.
  bzero((char *)&serv_addr, sizeof(serv_addr));
  serv_addr.sin_family = AF_INET;
  // Copy the server's IP address to the server address structure.
  bcopy((char *)server->h_addr, (char *)&serv_addr.sin_addr.s_addr,
        server->h_length);
  // Convert the port number integer to network big-endian style and save it to
  // the server address structure.
  serv_addr.sin_port = htons(portno);
  // Call up the server using its IP address and port number.
  if (connect(sockfd, (struct sockaddr *)&serv_addr, sizeof(serv_addr)) < 0)
    error("ERROR connecting");
  printf("\nNTP client started \n\n");

  while (1) {

    z = z + 1;
    ntp_packet packet = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
    memset(&packet, 0, sizeof(ntp_packet));
    *((char *)&packet + 0) =
        0x1b; // Represents 27 in base 10 or 00011011 in base 2.
    uint8_t *ptr = (uint8_t *)(&packet); /* to read raw bytes */
    gettimeofday(&tv1, NULL);
    unsigned long long millisecondsSinceEpochStart =
        (unsigned long long)(tv1.tv_sec) * 1000 +
        (unsigned long long)(tv1.tv_usec) / 1000;
    n = write(sockfd, (char *)&packet, sizeof(ntp_packet));

    if (n < 0)
      error("ERROR writing to socket");

    n = read(sockfd, (char *)&packet, sizeof(ntp_packet));
    if (n < 0)
      error("ERROR reading from socket");
    gettimeofday(&tv2, NULL);
    unsigned long long millisecondsSinceEpochEnd =
        (unsigned long long)(tv2.tv_sec) * 1000 +
        (unsigned long long)(tv2.tv_usec) / 1000;

    packet.precision = ntohl(packet.precision); // Precision

    packet.rootDelay = ntohl(packet.rootDelay);

    packet.rxTm_s = ntohl(packet.rxTm_s); // Time-stamp seconds.
    packet.rxTm_f = ntohl(packet.rxTm_f); // Time-stamp fraction of a second.

    packet.txTm_s = ntohl(packet.txTm_s); // Time-stamp seconds.
    packet.txTm_f = ntohl(packet.txTm_f); // Time-stamp fraction of a second.

    // packet.rootDelay = ntohl(packet.rootDelay); // RTT
    // time_t rootDelay = ( time_t ) ( packet.rootDelay - NTP_TIMESTAMP_DELTA );

    printf("\n................................................................."
           "...\n");
    printf("\nIteration Number : %d\n", z);
    printf("..................................................................."
           "..\n");
    printf("NTP Telegram Contains\n");
    for (i = 0; i < sizeof(ntp_packet); i++) {
      if (i != 0 && i % 8 == 0)
        printf("\n");
      printf("0x%2x ", ptr[i]);
    }
    printf("\n\n\n");

    printf("Client Send Timestamp T1(ms): %llu\n", millisecondsSinceEpochStart);

    printf("server Recieve Timestamp T2: %llu.", packet.rxTm_s);
    printf("%llu\n", packet.rxTm_f);

    printf("server Transmit Timestamp T3: %llu.", packet.txTm_s);
    printf("%llu\n", packet.txTm_f);

    printf("Client Recieve Timestamp T4(ms): %llu\n",
           millisecondsSinceEpochEnd);

    printf("RTT (calculated by NTP): %llu\n", packet.rootDelay);

    printf("Precision (calculated by NTP): %llu\n", packet.precision);

    sleep(atoi(argv[2]));
  }
  return 0;
  close(sockfd);
}

Makefile:

.PHONY: all clean tags

CFLAGS := -Wall -O2

all: client 

client: client.c
    $(CC) $(CFLAGS) $^ -o $@
    strip -s $@

tags:
    ctags -R .

clean:
    -rm *.o client 

make and run the code using

./client 0.pool.ntp.org 1

The problem I am facing and need your suggestions possible code changes and improvements

1. The code doesn't work infinitely, it stops in between and doesn't print any output after some iteration (no error message is shown)

2. The timestamps are not of same size although 64 bits

3. RTT and precision print the same value and doesn't get updated in each iteration (Sometimes both shows 0)

4. How to build T2 from packet.rxTm_s and packet.rxTm_f as one timestamp of 64bit.

Answer 1

When I compiled your code with some warnings flags, I got some warnings:

25 warnings generated.

Some are clearly not useful but some are really scary:

warning: implicit conversion changes signedness: 'int' to 'size_t' (aka 'unsigned long') [-Wsign-conversion]
        server->h_length);
        ~~~~~~~~^~~~~~~~
warning: declaration shadows a local variable [-Wshadow]
    ntp_packet packet = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
               ^
note: previous declaration is here
  ntp_packet packet = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
             ^
warning: declaration shadows a local variable [-Wshadow]
    uint8_t *ptr = (uint8_t *)(&packet); /* to read raw bytes */
             ^
note: previous declaration is here
  uint8_t *ptr = (uint8_t *)(&packet); /* to read raw bytes */
           ^
warning: implicit declaration of function 'gettimeofday' is invalid in C99 [-Wimplicit-function-declaration]
    gettimeofday(&tv1, NULL);
    ^
warning: implicit declaration of function 'write' is invalid in C99 [-Wimplicit-function-declaration]
    n = write(sockfd, (char *)&packet, sizeof(ntp_packet));
        ^
warning: implicit declaration of function 'read' is invalid in C99 [-Wimplicit-function-declaration]
    n = read(sockfd, (char *)&packet, sizeof(ntp_packet));
        ^
warning: implicit conversion loses integer precision: 'unsigned int' to 'uint8_t' (aka 'unsigned char') [-Wconversion]
    packet.precision = ntohl(packet.precision); // Precision
                     ~ ^~~~~~~~~~~~~~~~~~~~~~~
warning: format specifies type 'unsigned long long' but the argument has type 'uint32_t' (aka 'unsigned int') [-Wformat]
    printf("server Recieve Timestamp T2: %llu.", packet.rxTm_s);
                                         ~~~~    ^~~~~~~~~~~~~
                                         %u
warning: format specifies type 'unsigned long long' but the argument has type 'uint32_t' (aka 'unsigned int') [-Wformat]
    printf("%llu\n", packet.rxTm_f);
            ~~~~     ^~~~~~~~~~~~~
            %u
warning: format specifies type 'unsigned long long' but the argument has type 'uint32_t' (aka 'unsigned int') [-Wformat]
    printf("server Transmit Timestamp T3: %llu.", packet.txTm_s);
                                          ~~~~    ^~~~~~~~~~~~~
                                          %u
warning: format specifies type 'unsigned long long' but the argument has type 'uint32_t' (aka 'unsigned int') [-Wformat]
    printf("%llu\n", packet.txTm_f);
            ~~~~     ^~~~~~~~~~~~~
            %u
warning: format specifies type 'unsigned long long' but the argument has type 'uint32_t' (aka 'unsigned int') [-Wformat]
    printf("RTT (calculated by NTP): %llu\n", packet.rootDelay);
                                     ~~~~     ^~~~~~~~~~~~~~~~
                                     %u
warning: format specifies type 'unsigned long long' but the argument has type 'uint8_t' (aka 'unsigned char') [-Wformat]
    printf("Precision (calculated by NTP): %llu\n", packet.precision);
                                           ~~~~     ^~~~~~~~~~~~~~~~
                                           %hhu
warning: implicit declaration of function 'sleep' is invalid in C99 [-Wimplicit-function-declaration]
    sleep(atoi(argv[2]));
    ^
warning: implicit declaration of function 'close' is invalid in C99 [-Wimplicit-function-declaration]
  close(sockfd);
  ^
warning: unused parameter 'argc' [-Wunused-parameter]
int main(int argc, char *argv[]) {
             ^
warning: comparison of integers of different signs: 'int' and 'unsigned long' [-Wsign-compare]
    for (i = 0; i < sizeof(ntp_packet); i++) {
                ~ ^ ~~~~~~~~~~~~~~~~~~
warning: 'return' will never be executed [-Wunreachable-code-return]
  return 0;
         ^
warning: code will never be executed [-Wunreachable-code]
  close(sockfd);

I didn't keep them all but there are still 19! First of all you should fix all of them because they are important in my opinion, and ignoring those would likely cause undefined behavior. After you've cleaned all these warnings, you could try again to debug your code. (Try to get to zero warnings with clang -Weverything; not all warnings are useful but try to understand them before ignoring them).

---

Next, I tested your code and it seemed to work on my machine. Yet, there is a lot of potential undefined behavior in your code. Bit fields are quite inconvenient as their layout is implementation-defined.

Nevertheless, I get this output:

....................................................................
Iteration Number : 42
.....................................................................
NTP Telegram Contains
0x1c 0x 2 0x 3 0x 0 0xd7 0x 7 0x 0 0x 0
0x 0 0x 0 0x 8 0xad 0xf7 0x5c 0x9c 0x6b
0xdd 0xd1 0xe8 0x64 0xc1 0x31 0xc9 0x4a
0x 0 0x 0 0x 0 0x 0 0x 0 0x 0 0x 0 0x 0
0xca 0xe8 0xd1 0xdd 0x66 0x71 0xa4 0x25
0xca 0xe8 0xd1 0xdd 0xb8 0xb0 0xa6 0x25

Client Send Timestamp T1(ms): 1512536048971
server Recieve Timestamp T2: 3721521354.631533926
server Transmit Timestamp T3: 3721521354.631681208
Client Recieve Timestamp T4(ms): 1512536048973
RTT (calculated by NTP): 2007
Precision (calculated by NTP): 0

// after some iteration

....................................................................
Iteration Number : 69
.....................................................................
NTP Telegram Contains
0x1c 0x 2 0x 3 0x 0 0xd7 0x 7 0x 0 0x 0
0x 0 0x 0 0x 8 0xc7 0xf7 0x5c 0x9c 0x6b
0xdd 0xd1 0xe8 0x64 0xc1 0x31 0xc9 0x4a
0x 0 0x 0 0x 0 0x 0 0x 0 0x 0 0x 0 0x 0
0xe5 0xe8 0xd1 0xdd 0xcf 0x58 0x20 0x37
0xe5 0xe8 0xd1 0xdd 0x86 0xaf 0x22 0x37

Client Send Timestamp T1(ms): 1512536076040
server Recieve Timestamp T2: 3721521381.924866767
server Transmit Timestamp T3: 3721521381.925020038
Client Recieve Timestamp T4(ms): 1512536076042
RTT (calculated by NTP): 2007
Precision (calculated by NTP): 0

Which looks good to me.

---

So to answer to your questions:

1. The code didn't stop in my machine and worked as expected. Of course the reason why your code stopped is obvious: you have called write() and read() that could block until the I/O is completed. Remember that you are using UDP protocol, some packets will never arrive to the server or some packets from server will never reach you. If you want to code a real server, use something like select() or poll() or epoll().
2. NTP is at version 4, your code asks for 3 (0x1b; // Represents 27 in base 10 or 00011011 in base 2. => 3 for version field. By the way, you create a bit field... USE IT; don't use magic number). So I suppose that somehow you end up with a mix of version 3 and 4; the thing that I don't understand is that documentation of NTP claims the protocol to be backwards-compatible. To answer this question you should read the RFC V3 of NTP protocol (obsolete).
3. I have no idea of what is it and why it doesn't change.
4. I have no idea.

So, you will ask why do I answer you if I don't give you the answer to your question? It's because you've clearly misunderstood critical things.

First this tutorial is ******** (Linus' favorite word), you can't expect to parse a protocol with 30 lines of code in C when the last RFC of NTP protocol has 6163 lines ! You MUST read this protocol if you want to implement your own parser.

Secondly, the code of this tutorial is very badly written, my "wtf-o-matic" showed me a very high value of "wtf (worse than failure) by line". Why to use unsigned and not uint8_t for the bit field (bit fields are 80% implementation defined anyway) ? Why ntp_packet packet = { 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }; and not ntp_packet packet = {0}; ? Why and why and why use memset(&packet, 0, sizeof(ntp_packet)); just after ??? And what is that *((char *)&packet + 0) = 0x1b;, this IS ******** ! Well, there is more to say but I will just advice you to use getaddrinfo() in place of all the mess of gethostbyname(), htons() and etc...

---

As a conclusion: read the RFC that I linked (NTP V4); and stop using this "tutorial". If you want implement your own NTP library (the good news is coming), you could use the nice skeleton provided by the RFC, I was so happy to find a code example in a RFC that I made a gist of the example for you. And the good news is that the RFC is reduce by half in terms of documentation lines to read! So GL, HF in your lecture.

Answer 2

I've read the C code and agree about the "quality" of this code in terms of things to do.

For the analysis, I disagree on this point: - I don't think the problem is a real-time problem, this can be easily check by using wireshark and filter on the NTP protocol. Despite the fact, that the program fails, the wireshark should still capture data on the ethernet interface. - For me, the problem seems to be caused by the Kiss-o'-Death Packet (see section 7.4 of the RFC 5905). - A wireshark capture will help to know what really happens and investigate more deeply.