Date: Mon, 11 Nov 2002 09:22:58 -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: <200211111722.gABHMw3c033707@apollo.backplane.com> References: <3DC27247.5040100@wgate.com> <200211012008.gA1K8rOa034485@apollo.backplane.com> <3DCFD150.8080509@wgate.com>
next in thread | previous in thread | raw e-mail | index | archive | help
I was going to wait till 5.0 released first but I could do it now
if you want.
-Matt
:
:Matthew Dillon wrote:
:> 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?
:
:What is the status with this? As far as I can tell, the fix is correct
:and needed for some Java/JCK issues (the issue can be worked around in
:the JVM but that is the incorrect place to deal with 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?200211111722.gABHMw3c033707>