From owner-freebsd-net@FreeBSD.ORG Wed Jul 16 13:24:30 2014 Return-Path: Delivered-To: freebsd-net@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 761E1742 for ; Wed, 16 Jul 2014 13:24:30 +0000 (UTC) Received: from mail108.syd.optusnet.com.au (mail108.syd.optusnet.com.au [211.29.132.59]) by mx1.freebsd.org (Postfix) with ESMTP id 20330281B for ; Wed, 16 Jul 2014 13:24:29 +0000 (UTC) Received: from c122-106-147-133.carlnfd1.nsw.optusnet.com.au (c122-106-147-133.carlnfd1.nsw.optusnet.com.au [122.106.147.133]) by mail108.syd.optusnet.com.au (Postfix) with ESMTPS id 3E5DC1A2A81; Wed, 16 Jul 2014 23:24:21 +1000 (EST) Date: Wed, 16 Jul 2014 23:24:20 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Dmitry Sivachenko Subject: Re: does setsockopt(SO_RCVTIMEO) work? In-Reply-To: Message-ID: <20140716223814.O2597@besplex.bde.org> References: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=B4eAjodM c=1 sm=1 tr=0 a=7NqvjVvQucbO2RlWB8PEog==:117 a=PO7r1zJSAAAA:8 a=4dXZpFGpyrYA:10 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=MpIYFU-AUWHGpaUdnW4A:9 a=CjuIK1q_8ugA:10 Cc: freebsd-net@freebsd.org X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 16 Jul 2014 13:24:30 -0000 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 @ > 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