Mark,
On 11/22/21 11:57, Mark Thomas wrote:
On 22/11/2021 15:36, Christopher Schultz wrote:
Mark,
Apologies for top-posting but this might get lost in the code below.
No problem.
This patch appears to:
1. Get the address from the just-accepted socket
2. Compare the address to the most-recently-accepted socket address
(see #3)
2a. Throw an error if the current and previous address are the same
3. Save a reference to the just-accepted socket address
Won't this be a problem if there is only one connection being handled,
going in and out of the poll/accept cycle?
No. accept() only happens on initial connection.
Understood.
I haven't looked at the implementation of SocketAddress.equals and I
may be confusing things a bit, but I would imagine on a mostly-idle
server, this might cause an error:
1. Client connects from e.g. 127.0.0.1
2. Server accepts, stores 127.0.0.1
3. Client disconnects
4. Client connects from 127.0.0.1
2. Server accepts, throws error that 127.0.0.1 == 127.0.0.1
This would happen pretty frequently for connections through a
reverse-proxy.
Is there a way to differentiate between "I've seen this address
before" and "this is actually a duplicate?"
The address includes the remote port.
Aha, that should fix things, then. Adding the remote port should clear
everything up.
Looking at InetSocketAddress.equals is clear that the (client) port
number is indeed significant.
My observation is that the port that the client uses will increment for
subsequent connections so in the scenario above previous address will
not equal current address and Tomcat will continue as normal. >
You might be able construct a scenario where you could get a legitimate
duplication. My sense is that is even less likely that the bug so from
that point of view having the defensive code is better than not having it.
So since accept() is only called once per connection (regardless of how
many requests are processed through that connection), a client would
have to make a connection, close it, get the TCP/IP stack to re-use the
exact same local (outbound) port to connect to Tomcat, and *also* not
make any additional new connections (as to clear-away the
previously-accepted-socket address) in the meantime.
Yes, I agree this is ... unlikely.
The only situation I could see this happening in the real world would be
to have a client use SO_REUSEADDR+SO_REUSEPORT and make two connections
to each of two separate ports on the Tomcat server, with a shared
Acceptor thread. I'm fairly sure we don't ever share Acceptor threads in
Tomcat, so this situation is essentially impossible.
If we wanted to protect against legitimate duplication I think we'd have
to add some sort of timing constraint. i.e. the duplication has to occur
with some small number of milliseconds. I'm not sure how the
cost/benefit analysis would work out for that sort of change.
+1 to it not being worth it. We could store the number of microseconds
since since the last accept and allow a "repeat" if it's after a certain
amount of time, but clock behavior on systems isn't always reliable and
we'd never know how fast "too fast" might be.
Of course, the better option would be to get the bug fixed but my bug
report has been dormant since I raised it.
Right?
OTOH, whether the bug is fixed is less relevant than whether or not
admins accept the fix. :/
This workaround will probably have to remain in-place for the
foreseeable future.
Since this is a Linux-only bug, would it be worth it to make this
"tracking" optional, and only enable it on Linux JVMs? Or allow an
administrator to disable it if they know they have the patch?
It seems like very little overhead -- especially because it's done only
once per connection and not, for example once per request or (much
worse) once-per-IO-event. So it's probably not worth the extra code
unless it's easy to swap-out the implementation class of NIO[2]?Endpoint
with DuplicateDetectorEndpoint or something like that.
If it's easy to do that, it might be worth wrapping the Endpoint with a
decorator (requires significant refactoring) or using a subclass which
overrides serverSocketAccept().
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
