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

[James R T]

Hi Simon,

I have filled out the copyright assignment form accordingly.

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 <simon@josefsson.org> 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 <jamestiotio@gmail.com> 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 <jamestiotio@gmail.com>
> > ---
> >  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
> >               {