Date: Wed, 16 Jul 2014 11:02:43 -0700 From: Adrian Chadd <adrian@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: FreeBSD Net <freebsd-net@freebsd.org>, Dmitry Sivachenko <trtrmitya@gmail.com> Subject: Re: does setsockopt(SO_RCVTIMEO) work? Message-ID: <CAJ-VmomitYf6N=6tt4pno25VO8oS0KHGQMoobCZtS%2BNk%2BN%2BSBA@mail.gmail.com> In-Reply-To: <20140716223814.O2597@besplex.bde.org> References: <F6D31298-F070-4CB8-ADFF-243302A6CE1E@gmail.com> <20140716223814.O2597@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@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@freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-VmomitYf6N=6tt4pno25VO8oS0KHGQMoobCZtS%2BNk%2BN%2BSBA>