Re: [PATCH] traceroute: Fix hangs or delays in timeout

James R T <[email protected]> writes:

> Hi Simon,
>
> I have filled out the copyright assignment form accordingly.

Have you heard back from the FSF on this?  It would be nice to review
and merge your patch eventually.

/Simon

> The traceroute implementation by Dmitry Butskoy does not have this
> issue. Hence, it could be said that this patch would allow the
> traceroute implementation in `inetutils` to be more consistent with
> it. That said, no investigation has been done on how said
> implementation avoids or fixes this issue nor have I checked if this
> issue was ever brought up for discussion for that implementation in
> the first place.
>
> I also have not checked whether this issue is present in
> implementations other than the one written by Dmitry, such as Dublin
> Traceroute or Paris Traceroute.
>
> Best regards,
> James Raphael Tiovalen
>
> On Mon, Apr 24, 2023 at 3:12 PM Simon Josefsson <[email protected]> wrote:
>>
>> Hi.  Thank you -- I have sent you the copyright assignment form
>> separately.  Meanwhile, could you say something about how other
>> traceroute implementations behave here?  Does this patch make ours more
>> consistent with the rest, or is this an area where implementations
>> behave differently?
>>
>> /Simon
>>
>> James Raphael Tiovalen <[email protected]> writes:
>>
>> > Currently, traceroute resets the timeout to 3 seconds within the same
>> > probe when undesired ICMP traffic is received on the socket
>> > continuously, which causes it to hang or delay in timeout.
>> >
>> > This commit fixes this issue by setting the timeout to the remaining
>> > time within the same probe while processing the undesired ICMP packet
>> > instead of resetting the timeout to 3 seconds. The next probe is sent
>> > either when it times out or when the desired ICMP message is received.
>> >
>> > The fix can be verified by sending continuous ICMP traffic on the
>> > traceroute socket. Traceroute should not hang and there should not be
>> > any delays in the timeout.
>> >
>> > Signed-off-by: James Raphael Tiovalen <[email protected]>
>> > ---
>> >  src/traceroute.c | 35 +++++++++++++++++++++++++++--------
>> >  1 file changed, 27 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/src/traceroute.c b/src/traceroute.c
>> > index e96f2307..f0e8c242 100644
>> > --- a/src/traceroute.c
>> > +++ b/src/traceroute.c
>> > @@ -359,11 +359,18 @@ do_try (trace_t * trace, const int hop,
>> >    fd_set readset;
>> >    int ret, tries, readonly = 0;
>> >    struct timeval now, time;
>> > +  struct timeval start, end;
>> > +  long sec_elapsed = 0;
>> > +  long usec_elapsed = 0;
>> >    double triptime = 0.0;
>> >    uint32_t prev_addr = 0;
>> >
>> >    printf (" %2d  ", hop);
>> >
>> > +  memset (&time, 0, sizeof (time)); /* 64-bit issue. */
>> > +  memset (&start, 0, sizeof (start)); /* 64-bit issue. */
>> > +  memset (&end, 0, sizeof (end)); /* 64-bit issue. */
>> > +
>> >    for (tries = 0; tries < max_tries; tries++)
>> >      {
>> >        int save_errno;
>> > @@ -372,18 +379,19 @@ do_try (trace_t * trace, const int hop,
>> >        FD_ZERO (&readset);
>> >        FD_SET (fd, &readset);
>> >
>> > -      memset (&time, 0, sizeof (time));      /* 64-bit issue.  */
>> > -      time.tv_sec = opt_wait;
>> > -      time.tv_usec = 0;
>> > -
>> >        if (!readonly)
>> > -     trace_write (trace);
>> > +        {
>> > +          time.tv_sec = opt_wait;
>> > +          time.tv_usec = 0;
>> > +          trace_write (trace);
>> > +        }
>> >
>> >        errno = 0;
>> >        ret = select (fd + 1, &readset, NULL, NULL, &time);
>> >        save_errno = errno;
>> >
>> >        gettimeofday (&now, NULL);
>> > +      gettimeofday (&start, NULL);
>> >
>> >        now.tv_usec -= trace->tsent.tv_usec;
>> >        now.tv_sec -= trace->tsent.tv_sec;
>> > @@ -425,9 +433,20 @@ do_try (trace_t * trace, const int hop,
>> >             if (rc < 0)
>> >               {
>> >                 /* FIXME: printf ("Some error ocurred\n"); */
>> > -               tries--;
>> > -               readonly = 1;
>> > -               continue;
>> > +               gettimeofday (&end, NULL);
>> > +               sec_elapsed = (end.tv_sec - start.tv_sec);
>> > +               usec_elapsed = (end.tv_usec - start.tv_usec);
>> > +               if (usec_elapsed < 0)
>> > +                 usec_elapsed = 0;
>> > +               time.tv_sec -= sec_elapsed;
>> > +               time.tv_usec -= usec_elapsed;
>> > +               if (time.tv_sec >= 0 && time.tv_usec >= 0)
>> > +                 {
>> > +                   tries--;
>> > +                   if (!readonly)
>> > +                     readonly = 1;
>> > +                   continue;
>> > +                 }
>> >               }
>> >             else
>> >               {
>
>

-----BEGIN PGP SIGNATURE-----

iIoEARYIADIWIQSjzJyHC50xCrrUzy9RcisI/kdFogUCZOdeVRQcc2ltb25Aam9z
ZWZzc29uLm9yZwAKCRBRcisI/kdFooX3AQDGzMt6i9KMSlp83RMXMMi1HhVsTIr9
AsRSeN7XRRBHogEAnE5TF+z7sS6IT72n5dVGEQVSuG6TN3qpFyOGuMa7VQQ=
=k1vd
-----END PGP SIGNATURE-----