Re: ping

[Alain Magloire]

Bonjour

> I have just committed the last changes and integrated libicmp
> and ping into top-level Makefile.am and configure.in.

Cool.

> The current version of ping is able to send regular echo requests,
> timestamp and address mask requests. The problem with the latter is
> that its RFC definition seems completely broken, and most OS's simply
> discard ICMP_ADDRESS packets. I have implemented it anyway for the
> sake of completeness :^) ICMP_TIMESTAMP works fine.

> The router
> discovery and path MTU discovery requests will follow soon.

Cool, is this traceroute?

> 
> If you have some more cycles available, please try to compile and run it.

Testing on QNX/Neutrino, my linux box is still having disk problems
.. sigh..

1) Buglet
On Neutrino pid_t is 32 bit type and it is use fully 8-) here is a
small output of 'ps':
# ps
1307136042  1307136042     1  /usr/sbin/inetd
1307144235  1307136042     1  in.telnetd -Q
1307144236  1307144236  1307144236  -sh
1341390893  1307136042     1  in.telnetd -Q
1341390894  1341390894  1341390894  -sh
1342070831  1342070831  1341390894  ps

As you can see the pid_t numbers are not within the 16 bits range 8-)

So the bug is when you are comparing/saving/using the pid as an
'ident'ifier for the echo request.  In ping_init() you save the
pid in p->ping_ident, this is latter pass to initialise the icmp structure,
except that icmp->icmp_id field is a u_short(16 bits) the result will
be trucanted by the implicit cast.
When the ICMP_ECHOREPLY request comes back
ping_rcv(..) {
 case ICMP_ECHOREPLY:
      if (icmp->icmp_id != p->ping_ident)
        return -1;
}

it will fail.

It probably worked on Solaris, linux etc because the pid's always
fall within the 16 bit ranges on those systems.

Patch provided to explictily take the 16 bits:

+  /* Make sure we use only 16 bits in this field, id for icmp is a u_short.  */
+  p->ping_ident = ident & 0xFFFF;

2) <signal.h>
As stated in my previous message, there is no such thing as <sys/signal.h>
I mean we should rather use <signal.h> which is guarantee to exist.
QNX/Neutrino does not provide <sys/signal.h> and it is perfectly legal
and POSIX compliant ... this time I've checked 8-)

Patch provided.

3) buglet
icmp_address.c(icmp_addrss_encode)
The icmp should be initialise before accessing the icmp_mask field.

Patch provided.

4)
        icmp_cksum.c:29: warning: no previous prototype for `icmp_cksum'
Prototype missing for icmp_cksum().  I'm not sure you wanted
to export this or rather make static and only use in icmp_echo.c.

Not patch provided.

</Entering advocaty/religious mode no patch provided 8-) >

5)
        libping.c:313: warning: declaration of `sin' shadows global declaration 
'sin' is a bad name for a variable, it shadows a global name


6)
 Some inconsistencies in signess 8)

        libping.c:151: warning: comparison between signed and unsigned
        icmp_echo.c:65: warning: comparison between signed and unsigned

- 
alain

icmp.diff
Description: C program text