Date: Fri, 1 Nov 2002 12:08:53 -0800 (PST) From: Matthew Dillon <dillon@apollo.backplane.com> To: Michael Sinz <msinz@wgate.com> Cc: freebsd-hackers@FreeBSD.ORG Subject: Re: Socket so_linger setting Message-ID: <200211012008.gA1K8rOa034485@apollo.backplane.com> References: <3DC27247.5040100@wgate.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I think your patch is fine as is, Mike! Good find! Even though
so_linger cannot be negative, it is often convenient to use a signed
integer to store the value to avoid unexpected arithmatic results
when mixing with signed operations. My quick perusal does not show
any cases of this for so_linger, so we could make it unsigned, but I
don't see any pressing need to do so. The range check would need
to be in there in either case.
Can I go ahead and commit it?
-Matt
Matthew Dillon
<dillon@backplane.com>
:During some parameter limit checking work, I ran into what I believe to
:be an error in FreeBSD. (Albeit unlikely to be hit)
:
:A setsockopt of the SO_LINGER field will cause strange results if
:the value is set above 32767.
:
:This is due to the fact that in struct socket, the so_linger field
:is a signed short and the parameter passed to setsockopt for linger
:is a signed long.
:
:What happens is that any value between 32768 and 65535 will cause
:so_linger to be negative. And then getsockopt will return a sign
:extended negative value in the signed long field for linger.
:
:The "trivial" fix is to do the following:
:
:------------------------------------------------------
:--- uipc_socket.c Wed May 1 01:13:02 2002
:+++ /tmp/uipc_socket.c Fri Nov 1 06:55:10 2002
:@@ -1139,7 +1139,8 @@
: if (error)
: goto bad;
:
:- so->so_linger = l.l_linger;
:+ /* Limit the value to what fits in so_linger */
:+ so->so_linger = (l.l_linger > SHRT_MAX ? SHRT_MAX : l.linger);
: if (l.l_onoff)
: so->so_options |= SO_LINGER;
: else
:------------------------------------------------------
:
:What this does is limit the value to no more than 32767 (SHRT_MAX)
:However, I believe the more correct answer is that so_linger should
:not be a signed value to begin with.
:
:The reasoning is that what does a negative so_linger mean? To
:close the socket before the user does ;^)?
:
:It is somewhat obvious that so_linger does not need to be a long.
:
:It is not possible to change the API to make the input a short.
:
:Limiting the value to 32767 is reasonable (and that is a *vary*
:long linger time)
:
:However, given that negative linger values really don't exist
:(logically) it would be reasonable to not that field be signed.
:That would naturally limit the values to being within a valid
:range and prevent some strange results, specifically when
:looking at the tsleep() call where the so_linger field is
:just blindly multiplied by the hz of the system. (around line
:312 of uipc_socket.c) A segative so_linger will get sign extended
:into a negative int (32-bit) (times hz) and then passed to tsleep
:which just checks for zero, passed on to timeout which then
:passes it to callout_reset. It turns out that callout_reset will
:take negative values and make them a single tick... (whew! lucky
:thing that was there :-)
:
:The question I have is: should put together a patch that changes
:so_linger (and xso_linger) to unsigned? (And make sure there are
:no bad side effects) or is the trivial fix above what is wanted?
:
:[ My personal feeling is that since so_linger has no valid negative
: value that the field should be unsigned. Not that it matters
: about improving the range as 32767 is over 9 hours. It is more
: a matter of "correctness" in the code/representation since the
: code assumes the value is not negative already. ]
:
:--
:Michael Sinz -- Director, Systems Engineering -- Worldgate Communications
:A master's secrets are only as good as
: the master's ability to explain them to others.
To Unsubscribe: send mail to majordomo@FreeBSD.org
with "unsubscribe freebsd-hackers" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200211012008.gA1K8rOa034485>