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

[Simon Josefsson]

Thank you!  I was also thinking more how it compares to the "old"
traditional traceroute implementations found in BSD's.  I'll review the
code more carefully when the assignment is complete, and hopefully
merge it as-is.

/Simon

mån 2023-04-24 klockan 17:01 +0800 skrev 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
> > >               {

signature.asc
Description: This is a digitally signed message part