does setsockopt(SO_RCVTIMEO) work?

[Adrian Chadd]

I'm about to bump into this - would you be able to put together a
patch to address these issues? That way I can also test it out with
the UDP stuff I'm working on and get it into the tree.

Thanks,


-a


On 16 July 2014 06:24, Bruce Evans <brde at optusnet.com.au> wrote:
> On Wed, 16 Jul 2014, Dmitry Sivachenko wrote:
>
>> I am having trouble using {g,s}etsockopt(SO_RCVTIMEO).  Consider the
>> following small test program.
>> I expect to retrieve the value of 1 second via getsockopt call, I expect
>> the following output:
>> tv_sec=1, tv_usec=0
>> But I actually get
>> tv_sec=0, tv_usec=0
>>
>> What am I missing?
>>
>> Thanks!
>>
>> PS: on Linux it works as I expect.
>
>
> On old versions of FreeBSD it works as expected.
>
> This was broken last year.  I pointed out the bug and many others in my
> review, but it is still there.  From the review:
>
> @ On Sun, 1 Sep 2013, Davide Italiano wrote:
> @ @ > Log:
> @ >  Fix socket buffer timeouts precision using the new sbintime_t KPI
> instead
> @ >  of relying on the tvtohz() workaround. The latter has been introduced
> @ >  lately by jhb@ (r254699) in order to have a fix that can be backported
> @ >  to STABLE.
> @ >
> @ >  Reported by:       Vitja Makarov <vitja.makarov at gmail dot com>
> @ >  Reviewed by:       jhb (earlier version)
> @ @ This reintroduces overflow bugs that were fixed by using tvtohz().
> tvtohz()
> @ clamps the value to INT_MAX instead of overflowing, but tvtosbt() just
> @ overflows.
>
> These bugs are small (they are are only for timevals larger than INT_MAX
> seconds, which can only occur on systems with bloated 64-bit time_t's),
> but they were fixed long ago.
>
> @ @ This gives much larger overflow bugs in getsockopt().
>
> This bug is still there.  So in your test program, the timeout is set
> correctly, but it is returned as 0 after overflow of a large value
> gives 0.  All values larger than 1 second are truncated to less than
> 1 second.
>
> I expected more garbage in the values for timevals between 0.5 seconds
> (inclusive) and 1.0 seconds (not inclusive).  I think they would happen
> for the corresonding bug in setsockopt(), but in getsockopt() there is
> only a small (but annoying) rounding error.  I tried a timeval of
> 0 seconds, 500000 microseconds.  This was returned as 0 seconds,
> 499999 microseconds.  This rounding error happens because almost no values
> in the power-of-10 time scale for timevals can be represented in the
> power-of-2 time scale for sbintimes.  The rounding is always down, so
> conversion back and forth drops 1 ULP in almost all cases.  bintime
> gives similar errors.  sys/time.h has a large comment defending this
> bug.
>
> @ @ > ...
> @ > Modified: head/sys/kern/uipc_socket.c
> @ >
> ==============================================================================
> @ > --- head/sys/kern/uipc_socket.c     Sun Sep  1 23:06:28 2013
> (r255137)
> @ > +++ head/sys/kern/uipc_socket.c     Sun Sep  1 23:34:53 2013
> (r255138)
> @ > @@ -2541,7 +2541,7 @@ sosetopt(struct socket *so, struct socko
> @ >     int     error, optval;
> @ >     struct  linger l;
> @ >     struct  timeval tv;
> @ > -   u_long  val;
> @ > +   sbintime_t val;
> @ >     uint32_t val32;
> @ > #ifdef MAC
> @ >     struct mac extmac;
> @ @ This fixes a style bug (type error) that should have been fixed in the
> @ previous commit.  'int optval' is used for most options, but the old
> @ code needed `u_long val' for its home made conversion.  u_long became
> @ unnecessary and the extra variable became bogus when tvtohz() was used,
> @ since optval can hold tvtohz().
> @ @ > @@ -2703,7 +2703,7 @@ sosetopt(struct socket *so, struct socko
> @ >                             error = EDOM;
> @ >                             goto bad;
> @ >                     }
> @ @ There used to be more range checking above here.  Some was moved into
> @ tvtohz().  Changing to tvtosbt() moves it to /dev/null.
> @ @ > -                 val = tvtohz(&tv);
> @ > +                   val = tvtosbt(tv);
> @ >
> @ >                     switch (sopt->sopt_name) {
> @ >                     case SO_SNDTIMEO:
> @ @ optval can't hold tvtosbt(), so an extra variable is not a large style
> @ bug now.  But it can be avoided here by doing 2 assignments of tvtosbt()
> @ to so_snd/rcv.sb_timeout.
>
> 'val' _can_ hold tvtosbt() (it was enlarged to do this), so there is only
> a far-off overflow bug here (when tvtosbt() overflows internally)).
>
> @ @ > @@ -2857,8 +2857,7 @@ integer:
> @ >                     optval = (sopt->sopt_name == SO_SNDTIMEO ?
> @ >                               so->so_snd.sb_timeo :
> so->so_rcv.sb_timeo);
> @ @ This is now very broken.  optval is only int, so it can't hold the
> 64-bit
> @ sbintime_t.  I think it can hold times less than 0.5 seconds.  Overflow
> @ starts at 0.5 seconds by giving sign extension bugs on 2's complement
> machines.
> @ @ Style bug: non-KNF indentation.
>
> This still has all its bugs and style bugs.
>
> @ @ >
> @ @ Style bug: extra blank line separates related code.
> @ @ > -                 tv.tv_sec = optval / hz;
> @ > -                   tv.tv_usec = (optval % hz) * tick;
> @ > +                   tv = sbttotv(optval);
> @ @ This together with the style can be fixed by 2 assignments also (1
> @ assignment in the conditional operater also fixes verbosenes):
> @ @                     tv = sbttotv(sopt->sopt_name == SO_SNDTIMEO ?
> @                           so->so_snd.sb_timeo : so->so_rcv.sb_timeo);
>
> Fix for the main bug some style bugs.
>
> [... 200 more lines with further duscussion of style and overflow bugs]
>
> Bruce
>
> _______________________________________________
> freebsd-net at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"