Date: Wed, 16 Jul 2014 23:24:20 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Dmitry Sivachenko <trtrmitya@gmail.com> Cc: freebsd-net@freebsd.org Subject: Re: does setsockopt(SO_RCVTIMEO) work? Message-ID: <20140716223814.O2597@besplex.bde.org> In-Reply-To: <F6D31298-F070-4CB8-ADFF-243302A6CE1E@gmail.com> References: <F6D31298-F070-4CB8-ADFF-243302A6CE1E@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140716223814.O2597>