RFC: Net::SNTP::Server v1

[thanos1983]

Dear Monks,

Once again I need your expertise opinion as I am about to upload my second module related with Net::SNTP::Client.

The module serves purpose of a simple SNTP server able to reply back to client requests based RFC4330 message format.

In order to test the code I will provide both the client and server scripts so you can easily test and observe bugs or possible improvements based on your experience.

Update: changing value(s) from "0" to "1" -RFC4330 => "1" and -clearScreen => "1" on the client code.

Update 2: Modifying POD based on new findings, also the function $verify_port and last adding a new error message/restriction in checking $moduleInput{-port}.

Update 3: Modifying server module based on comments from Monk::Thomas.

Update 4: Modifying server module based on comments from Anonymous Monk.

Client script:

#!/usr/bin/perl
use strict;
use warnings;
use Data::Dumper;
use Net::SNTP::Client qw ( getSNTPTime );

my %hashInput = (
    -hostname      => "127.0.0.1",
    -port          => 12345,
    -RFC4330       => "1",
    -clearScreen   => "1",
    );

my ( $error , $hashRefOutput ) = getSNTPTime( %hashInput );

print Dumper $hashRefOutput;
print "Error: $error\n" if ($error);

Server script:

#!/usr/bin/perl
use strict;
use warnings;
use Data::Dumper;

use lib '/home/user/Desktop/Net-SNTP-Server-0.01/lib/'; # note here
use Net::SNTP::Server qw ( basicSNTPSetup );

my %hashInput = (
    -ip      => "127.0.0.1",
    -port    => 12345,
    );

my ( $error , $hashRefOutput ) = basicSNTPSetup( %hashInput );

print Dumper $hashRefOutput;
print "Error: $error\n" if ($error);

Server module:

package Net::SNTP::Server;

## Validate the version of Perl

BEGIN { die 'Perl version 5.6.0 or greater is required' if ($] < 5.006
+); }

use strict;
use warnings;

=head1 NAME

Net::SNTP::Server - Perl Module SNTP Server based on L<RFC4330|https:/
+/tools.ietf.org/html/rfc4330>

=head1 VERSION

Version 0.01

=cut

## Version of the Net::SNTP::Server module

our $VERSION = '0.01';
$VERSION = eval $VERSION;

use IO::Socket::INET;
use Time::HiRes qw( gettimeofday CLOCK_REALTIME clock_getres );

## Handle importing/exporting of symbols

use Exporter 5.57 qw( import ); # Because I am using an earlier versio
+n of Perl 5.8.2 
our @EXPORT_OK = qw( basicSNTPSetup );  # symbols to export on request

=head1 SYNOPSIS

The Net::SNTP::Server - Perl module has the ability to retrieve the
 time from the local internal clock of the OS. The module has been 
 tested on LinuxOS but it should be compatible with MacOS and WindowsO
+S.
 When the server is activated, it will enter while state mode and wait
 for client requests. The SNTP server uses the local time in seconds a
+nd 
 nano seconds accuracy based on the OS accuracy ability. The server wi
+ll
 encode a message format based on RFC4330 that will be send to the cli
+ent 
 in-order to calculate the round-trip delay d and system clock offset.

    use Net::SNTP::Server;

    my ( $error , $hashRefOutput ) = basicSNTPSetup( %hashInput );
    ...

=head1 ABSTRACT

The module receives and sends a UDP packet formated according to
 L<RFC4330|https://tools.ietf.org/html/rfc4330> message format.
 The server expects an SNTP packet from the client which will reply 
 back to him. The received packet, gets decoded into a human readable
 form. As a second step the server extracts and adds the needed
 data to create the packet. Before the message to be sent to the clien
+t
 it gets encoded and transmitted back to the recipient.


=head1 DESCRIPTION

This module exports a single method (basicSNTPSetup) and returns
 an associative hash based on the user input and a string in case
 of an error. The response from the SNTP server is been encoded
 to a human readable format. The obtained information received
 from the server on the client side can be used into further processin
+g
 or manipulation according to the user needs. Maximum accuracy down
 to nano seconds can only be achieved based on different OS.

=head2 EXPORT

  my %hashInput = (
      -ip      => $ip,    # IP
      -port    => $port,  # default NTP port 123 
      );

  my ( $error , $hashRefOutput ) = basicSNTPSetup( %hashInput );

=over 4

=item * IP

    -ip: Is not a mandatory for the method key to operate correctly.
    By default the module will assign the localhost IP ("127.0.0.1"),
    but this will restrict the server to localhost communications (onl
+y
    internally it can receive and transmit data).


=item * PORT

    -port: Is a mandatory key, that the user must define. By default t
+he
    port is not set. The user has to specify the port. We can not use 
+the
    default 123 NTP due to permission. The user has to choose a port n
+umber
    identical to port that the client will use client to communicate w
+ith
    the server (e.g. -port => 123456).

=back

=head1 SUBROUTINES/METHODS

  my ( $error , $hashRefOutput ) = basicSNTPSetup( %hashInput );


=cut

## Define constands

use constant {
    TRUE                  => 1,
    FALSE                 => 0,
    MAXBYTES              => 512,
    ARGUMENTS             => 1,
    UNIX_EPOCH            => 2208988800,
    MIN_UDP_PORT          => 1,
    MAX_UDP_PORT          => 65535,
    DEFAULT_LOCAL_HOST_IP => "127.0.0.1",
};

sub basicSNTPSetup {
    my $error = undef;
    my $rcvSntpPacket = undef;
    my ( $rcv_sntp_packet ,  $server_precision );

    my %moduleInput = @_;
    my %moduleOutput = ();

    return ($error = "Not defined IP", \%moduleInput) if (!$moduleInpu
+t{-ip});
    return ($error = "Not defined Port", \%moduleInput) if (!$moduleIn
+put{-port});
    return ($error = "Not defined key(s)", \%moduleInput) if (checkHas
+hKeys(%moduleInput));
    return ($error = "Not correct port number", \%moduleInput) if (ver
+ify_port($moduleInput{-port}));
    
    my ( @array_IP ) = ( $moduleInput{-ip} =~ /(\d{1,3})\.(\d{1,3})\.(
+\d{1,3})\.(\d{1,3})/ );
    return ($error = "Not correct input IP syntax", \%moduleInput) 
    if ( (!defined $array_IP[0]) || 
         (!defined $array_IP[1]) || 
         (!defined $array_IP[2]) || 
         (!defined $array_IP[3]) );

    # Convert IP
    my $server_reference_identifier_hex .= dec_2_hex( @array_IP );

    my $server_socket;
    eval {
    $server_socket = IO::Socket::INET->new(
        LocalAddr  => $moduleInput{-ip} || DEFAULT_LOCAL_HOST_IP,
        LocalPort  => $moduleInput{-port}, # Mandatory, we can't use N
+TP default 123
        Proto      => 'udp',
        Type       => SOCK_DGRAM,
        Broadcast  => 1 ) or die "Error Creating Socket"; 
    };
    return ($error = "Problem While Creating Socket '$!'", \%moduleInp
+ut) if ( $@ && $@ =~ /Error Creating Socket/ );
    
    print "\n[Server $0 listens at PORT: ".$moduleInput{-port}." and I
+P: ".$moduleInput{-ip}."]\n\n";

    while ( TRUE ) {
    # Reference Timestamp: This field is the time the system clock was
+ last
    # set or corrected, in 64-bit timestamp format. (assumption of tim
+e synchronization)
    my ( $server_reference_timestamp_sec,
         $server_reference_timestamp_microsec ) = gettimeofday();

    my $peer_address = $server_socket->peerhost();
    my $peer_port = $server_socket->peerport();
    
    if ( $peer_address ) { print "Peer address: ".$peer_address."\n" }
+;
    if ( $peer_port ) { print "Peer port: ".$peer_port."\n" };
    
    eval {
        $server_socket->recv( $rcv_sntp_packet , MAXBYTES )
        or die "Error Receiving";
    };
    return ($error = "Problem While Receiving '$!'", \%moduleInput) if
+ ( $@ && $@ =~ /Error Receiving/ );
    
    # $server_rcv_timestamp_sec (originate time rcv by server sec) # 3
+2 bit
    # $server_rcv_timestamp_microsec (originate time rcv by server mic
+rosec) # 32 bit
    my ( $server_rcv_timestamp_sec, 
         $server_rcv_timestamp_microsec ) = gettimeofday();

    my @arrayRcvSntpPacket = unpack( "B8 C3 N11" , $rcv_sntp_packet );

    my ( $client_li_vn_mode,
         $client_stratum,
         $client_poll,
         $client_precision,
         $client_root_delay,
         $client_dispersion,
         $client_reference_identifier,
         $client_reference_timestamp_sec,
         $client_reference_timestamp_microsec,
         $client_originate_timestamp_sec,
         $client_originate_timestamp_microsec,
         $client_receive_timestamp_sec,
         $client_receive_timestamp_microsec,
         $client_transmit_sec,
         $client_transmit_microsec ) = @arrayRcvSntpPacket;
    
        # Server data preparing message
    # $li = 00 2 bit = value 0 no warning
    # $vn = 100 3 bit = Value 4 IPV4
    # $mode = 100 3 bit = Value 4 server mode
    my $server_li_vn_mode = '00100100';
    my $server_stratum = '00000010'; # Stratum is 3 because
    my $server_poll_interval = 3; # Poll interval is 6

    # The precission size on the RFC is 8 bits, anything lower
    # than 1e-03 (0.001) it can not fit on 8 bits. In such cases
    # we round to clossest digit. Never the less the value is so
    # small not even worth mentioning it.

    if ( $^O eq 'MSWin32' ) {
        ( undef , undef ,  $server_precision , undef , undef ) = POSIX
+::times() ;
    }
    else {
        $server_precision = clock_getres( CLOCK_REALTIME );
    }

    my $server_root_delay_sec = '0'; # 16 bit
    my $server_root_delay_microsec = '0'; # 16 bit
    my $server_dispersion_sec = '0'; # 16 bit
    my $server_dispersion_microsec = '0'; # 16 bit

    $server_reference_timestamp_sec += UNIX_EPOCH;
    $server_rcv_timestamp_sec += UNIX_EPOCH;

    # $server_transmit_timestamp_sec (originate time rcv by server sec
+) # 32 bit
    # $server_transmit_timestamp_microsec (originate time rcv by serve
+r microsec) # 32 bit
    my ( $server_transmit_timestamp_sec,
         $server_transmit_timestamp_microsec ) = gettimeofday();

    $server_transmit_timestamp_sec += UNIX_EPOCH;

    my @arraySendSntpPacket = ( $server_li_vn_mode,
                    $server_stratum,
                    $server_poll_interval,
                    $server_precision,
                    $server_root_delay_sec, 
                    $server_root_delay_microsec,
                    $server_dispersion_sec,
                    $server_dispersion_microsec,
                    $server_reference_identifier_hex,
                    $server_reference_timestamp_sec,
                    $server_reference_timestamp_microsec,
                    $client_transmit_sec,
                    $client_transmit_microsec,
                    $server_rcv_timestamp_sec,
                    $server_rcv_timestamp_microsec,
                    $server_transmit_timestamp_sec,
                    $server_transmit_timestamp_microsec );
    
    my $send_sntp_packet = pack( "B8 C3 n B16 n B16 H8 N8" , @arraySen
+dSntpPacket );

    eval {
        $server_socket->send( $send_sntp_packet )
        or die "Error Sending";
    };
    return ($error = "Problem While Sending '$!'", \%moduleInput) if (
+ $@ && $@ =~ /Error Sending/ );

    } # End of while(TRUE) loop

    $server_socket->close(); # Close socket

    return $error, \%moduleOutput;
}

sub dec_2_hex {
    my ( @decimal_IP ) = @_;
    my $hex = join(
    '', map { sprintf '%02X', $_ } $decimal_IP[0], $decimal_IP[1], $de
+cimal_IP[2], $decimal_IP[3]);
    return ( uc( $hex ) );
}

sub checkHashKeys {
    my @keysToCompare = ( "-ip", "-port" );
    my %hashInputToCompare = @_;
    my @hashInputKeysToCompare = keys %hashInputToCompare;

    local *keyDifference = sub {
    my %hashdiff = map{ $_ => 1 } @{$_[1]}; 
    return grep { !defined $hashdiff{$_} }  @{$_[0]};
    };
    my @differendKeys = keyDifference(\@hashInputKeysToCompare, \@keys
+ToCompare);
    
    # c - style if condition
    return TRUE ? @differendKeys : return FALSE;
    # Or if (@differendKeys) { return TRUE } else { return FALSE };
}

sub verify_port {
    my $port = shift;
    if ( defined $port && $port =~ /^[0-9]+$/ ) {
    if ( $port >= MIN_UDP_PORT && MAX_UDP_PORT >= $port ) {
        return FALSE;
    }
    }
    return TRUE;
}

=head1 EXAMPLE

This example starts a remote NTP server based on RFC4330 message forma
+t. 
 The IP and Port that the user will provide based on his criteria.

We use the L<Data::Dumper|http://search.cpan.org/~ilyam/Data-Dumper-2.
+121/Dumper.pm>
 module to print the output if needed. The module does not require
 to printout the output. It should be used only for initialization 
 purposes to assist the user with debugging in case of an error. 
 The $error string it is also optional that will assist the user
 to identify the root that can cause a faulty initialization.


  #!/usr/bin/perl
  use strict;
  use warnings;
  use Data::Dumper;

  use Net::SNTP::Server qw(basicSNTPSetup);

  my %hashInput = (
      -ip      => "127.0.0.1",
      -port    => 12345,
      );

  my ( $error , $hashRefOutput ) = basicSNTPSetup( %hashInput );

  print Dumper $hashRefOutput;
  print "Error: $error\n" if ($error);


=head1 AUTHOR

Athanasios Garyfalos, C<< <garyfalos at cpan.org> >>

=head1 BUGS

Please report any bugs or feature requests to C<bug-net-sntp-server at
+ rt.cpan.org>, or through
the web interface at L<http://rt.cpan.org/NoAuth/ReportBug.html?Queue=
+Net-SNTP-Server>. I will be notified, and then you'll
automatically be notified of progress on your bug as I make changes.




=head1 SUPPORT

You can find documentation for this module with the perldoc command.

    perldoc Net::SNTP::Server


You can also look for information at:

=over 4

=item * RT: CPAN's request tracker (report bugs here)

L<http://rt.cpan.org/NoAuth/Bugs.html?Dist=Net-SNTP-Server>

=item * AnnoCPAN: Annotated CPAN documentation

L<http://annocpan.org/dist/Net-SNTP-Server>

=item * CPAN Ratings

L<http://cpanratings.perl.org/d/Net-SNTP-Server>

=item * Search CPAN

L<http://search.cpan.org/dist/Net-SNTP-Server/>

=back


=head1 ACKNOWLEDGEMENTS

I want to say thank you to L<Perl Monks The Monastery Gates|http://www
+.perlmonks.org/> 
 for their guidance and assistance when ever I had a problem with
 the implementation process of module.

=head1 LICENSE AND COPYRIGHT

Copyright 2015 Athanasios Garyfalos.

This program is free software; you can redistribute it and/or modify i
+t
under the terms of the the Artistic License (2.0). You may obtain a
copy of the full license at:

L<http://www.perlfoundation.org/artistic_license_2_0>

Any use, modification, and distribution of the Standard or Modified
Versions is governed by this Artistic License. By using, modifying or
distributing the Package, you accept this license. Do not use, modify,
or distribute the Package, if you do not accept this license.

If your Modified Version has been derived from a Modified Version made
by someone other than you, you are nevertheless required to ensure tha
+t
your Modified Version complies with the requirements of this license.

This license does not grant you the right to use any trademark, servic
+e
mark, tradename, or logo of the Copyright Holder.

This license includes the non-exclusive, worldwide, free-of-charge
patent license to make, have made, use, offer to sell, sell, import an
+d
otherwise transfer the Package with respect to any patent claims
licensable by the Copyright Holder that are necessarily infringed by t
+he
Package. If you institute patent litigation (including a cross-claim o
+r
counterclaim) against any party alleging that the Package constitutes
direct or contributory patent infringement, then this Artistic License
to you shall terminate on the date that such litigation is filed.

Disclaimer of Warranty: THE PACKAGE IS PROVIDED BY THE COPYRIGHT HOLDE
+R
AND CONTRIBUTORS "AS IS' AND WITHOUT ANY EXPRESS OR IMPLIED WARRANTIES
+.
THE IMPLIED WARRANTIES OF MERCHANTABILITY, FITNESS FOR A PARTICULAR
PURPOSE, OR NON-INFRINGEMENT ARE DISCLAIMED TO THE EXTENT PERMITTED BY
YOUR LOCAL LAW. UNLESS REQUIRED BY LAW, NO COPYRIGHT HOLDER OR
CONTRIBUTOR WILL BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, OR
CONSEQUENTIAL DAMAGES ARISING IN ANY WAY OUT OF THE USE OF THE PACKAGE
+,
EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.

=head1 CHANGE LOG

$Log: Server.pm,v $
Revision 1.0  2015/07/17 16:32:31  Thanos


=cut

1; # End of Net::SNTP::Server

Thank you all in advance for your time and effort reviewing my work.

[Monk::Thomas]

1) know your tools

    # Convert the decimal to hexadecimal and pad the result with a lea
+ding
    # zero if needed.
    my $hex = sprintf("%x", $dec);
    $hex = length( $hex ) == 1 ? "0".$hex : $hex;

way too complicated

    my $hex = sprintf '%02x', $dec;

conversion table

  0 -> 00
  1 -> 01
 15 -> 0f
 20 -> 14
255 -> ff
256 -> 100    # garbage in, garbage out

2) nesting subs does not work:

sub basicSNTPSetup {
  ...   
  sub keyDifference {
    ..
  };
};

is equivalent to

sub basicSNTPSetup {
  ...   
};

sub keyDifference {
  ..
};

3) dead code / sub-ref / conditional

my $verify_port = sub {
  my $port = shift;
  if ( defined $port && $port =~ /^[0-9]+$/ ) {
    if ( $port >= MIN_UDP_PORT || $port <= MAX_UDP_PORT ) {
      return FALSE;
    }
  }
  return TRUE;
};

This functions does not seem to be referenced anywhere in your code. Additionally: Why do you code it as a sub reference instead of an actual function?

sub verify_port {
  ...
};

inside verify_port you are using the wrong conditional

  if ( $port >= MIN_UDP_PORT || $port <= MAX_UDP_PORT ) {

This will happily allow 10000000000 as a valid port. At the very least you should replace || with &&. I'm to lazy too look up operator precedence, but you may also need to replace && with 'and' or put the conditions into parenthesis.

either:
  if ( ($port >= MIN_UDP_PORT) && ($port <= MAX_UDP_PORT) ) {
or:
  if ( $port >= MIN_UDP_PORT and $port <= MAX_UDP_PORT ) {

Resumee:

I would not want to touch this module with a ten-foot pole. I wasn't even looking very hard.

[thanos1983]

Hello Monk::Thomas,

First of all thank you for your time and effort reading and reviewing my code. I am not an expert and it is good that you point out this error to me so I can modify the code and updated to a better hopefully version.

Regarding Nested Functions, I found this tutorial Creating Nested Functions, from where I modified my functions to local hopefully this is better now and more memory efficient.

Regarding your comment:

I would not want to touch this module with a ten-foot pole. I wasn't even looking very hard.

Is is to so badly written the module? What is so wrong that would make you not want to use this module?

Again, thank you for time and appreciate your effort.

[Monk::Thomas]

Regarding Nested Functions, I found this tutorial Creating Nested Functions, from where I modified my functions to local hopefully this is better now and more memory efficient.

If you're concerned about optimized memory consumption for an 8 line function, then you probably should use something else. (e.g. finely tuned C code) My recommendation: Optimize for readability / maintainability. You like to publish the module so that someone else might use it. This someone else might have an anger management problem, an axe and your address. ;)

Is is to so badly written the module? What is so wrong that would make you not want to use this module?

It's trust related. If I use someone else's code then I must be sure the code is doing what it's supposed to do. Easy to understand code and suitable application of common language idioms help to build that trust. (Hey! I can understand that!) Automated tests are also nice. (Hey! I don't understand it, but it's seems to be working as expected.) Your code simply fails to build that trust.

'Three strikes and you are out:'

1. Trying to reinvent printf, especially even though you already use it.
2. Code that does not seem to be used.
3. Failing to validate user input.

No trust -> that code is not going to end up on any of my machines. I may want to play around with it for educational purposes, but it's not going to be something I rely on.

[Anonymous Monk]

I have so far only read most of the code, not the documentation. Here are a couple of issues I noticed with the code. (Code snippets are untested.)

1. BEGIN { die 'Perl version 5.6.0 or greater is required' if ($] < 5.006); } can be replaced by use 5.006;. Also, unless perlver is mistaken, the minimum required version for your code is v5.8.0.
2. use base qw( Exporter ); our @ISA = qw ( Exporter );: You don't need to use base and modify @ISA. Also, that code can be replaced by simply use Exporter "import";; or, if you really want to inherit from Exporter, use parent instead of base.
3. $dot is used only once, and is so simple that it should be inlined.
4. Code like local *checkHashKeys = sub {: Don't do that without a good reason (and I don't see one). Use a "private" sub instead (a regular sub whose name begins with an underscore and that isn't exported). Several of your subs of this form are used only once and can and probably should be inlined.
5. Your dec_2_hex can be replaced by sprintf('%02X%02X%02X%02X',$a,$b,$c,$d) or maybe join('',map {sprintf '%02X', $_} $a,$b,$c,$d) (note the uppercase X).
6. Code like return ($error = "Not defined IP", \%moduleInput): $error will go out of scope immediately after the return, so assigning to it is not useful and confusing to readers, since it's not immediately apparent whether $error might be a package variable or not (which would actually make more sense than it being a lexical, as it currently is).
7. Error handling in general: While the return values of your sub are certainly your choice, what you've chosen is uncommon. Perhaps you should be throwing errors with croak (Carp; that's how I'd likely do it), or, if you don't want to throw errors, return nothing (return;) and set a special error variable.
8. Lots of unused variables, and several variables used only once like @arraySendSntpPacket that can be inlined.
9. The body of your main while loop is not indented properly and you have several very long lines that should probably be wrapped.

Will you be releasing tests with this module?

[thanos1983]

Hello Anonymous,

Thank you for reviewing my code.

Well I am using (5.006) because in the source code of Time::HiRes, the author using this version. Since I am using the module I assumed I should not have higher version. The die syntax I picked it up from the Net::SNMP source code. I thought it is more tricky and nice to write it like this.

The only reason that I am using named subroutines, is to assist my self why I did something in future. I know that most of my functions only apply once in the code, but I assumed that this is the way to remember what I was trying to do in future.

Unused Variables such as @arraySendSntpPacket etc. could be inlined, but they help me to debug the code easier than defining all the variables one by one and printing them. Also for future when I will be updating the code it would make it easier to remember why e.g. a variable is 0 or 3 because it is li _vn_mode.

Yes I have prepared a few tests that cam to my mine with the basic version that I want to release. I am planning to release also a bit more complicated version updating the internal clock based on user preference. One step at a time.

[syphilis]

Since I am using the module I assumed I should not have higher version

I can see how one might arrive that conclusion, but it's incorrect.
"use 5.006;" is there to ensure that 5.006 or higher is in use - that's it's only intent.
IMO it's an unnecessary stipulation, and it certainly doesn't mean that you should use perl 5.6.

Cheers,
Rob