On Tue, Apr 10, 2018 at 11:11:39PM +0300, Vadim Zhukov wrote:
> Hi all!
> 
> While working on home job for students, I've come across two
> questionable thingies in ping.c:
> 
> 1. It sends process PID (well, last 16 bits) to the network.
>    Maybe I'm a bit paranoid, but this looks like bad idea for me.
>    I understand that this worked good when PIDs were 16-bit limited,
>    because in that case you'll get 100% guarantee two different
>    ping processes won't overlap. But nowadays we have larger PID
>    space, so clashes are possible anyway. I propose to go straight
>    with arc4random().
> 
> 2. There is no point in calling ntohs/htons on the ident value.
>    Or we should do this in IPv4 too, then: I'm not opposed, but
>    at least consistency should be honored, IMHO.
> 
> Okay? Comments?

OK florian@

> 
> --
> WBR,
>   Vadim Zhukov
> 
> P.S.: I'm not fully back yet, sorry.
> 
> 
> Index: ping.c
> ===================================================================
> RCS file: /cvs/src/sbin/ping/ping.c,v
> retrieving revision 1.224
> diff -u -p -r1.224 ping.c
> --- ping.c    8 Nov 2017 17:27:39 -0000       1.224
> +++ ping.c    10 Apr 2018 20:03:50 -0000
> @@ -586,7 +586,7 @@ main(int argc, char *argv[])
>               for (i = ECHOTMLEN; i < datalen; ++i)
>                       *datap++ = i;
>  
> -     ident = getpid() & 0xFFFF;
> +     ident = arc4random() & 0xFFFF;
>  
>       /*
>        * When trying to send large packets, you must increase the
> @@ -1033,7 +1033,7 @@ pinger(int s)
>               icp6->icmp6_cksum = 0;
>               icp6->icmp6_type = ICMP6_ECHO_REQUEST;
>               icp6->icmp6_code = 0;
> -             icp6->icmp6_id = htons(ident);
> +             icp6->icmp6_id = ident;
>               icp6->icmp6_seq = seq;
>       } else {
>               icp = (struct icmp *)outpack;
> @@ -1151,7 +1151,7 @@ pr_pack(u_char *buf, int cc, struct msgh
>               }
>  
>               if (icp6->icmp6_type == ICMP6_ECHO_REPLY) {
> -                     if (ntohs(icp6->icmp6_id) != ident)
> +                     if (icp6->icmp6_id != ident)
>                               return;                 /* 'Twas not our ECHO */
>                       seq = icp6->icmp6_seq;
>                       echo_reply = 1;
> 

-- 
I'm not entirely sure you are real.

Reply via email to