You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2016/02/01 20:47:14 UTC
svn commit: r1727992 - in /tomcat/trunk:
java/org/apache/tomcat/util/net/SecureNioChannel.java
webapps/docs/changelog.xml
Author: markt
Date: Mon Feb 1 19:47:13 2016
New Revision: 1727992
URL: http://svn.apache.org/viewvc?rev=1727992&view=rev
Log:
Fix a consistent unit test failure on OSX (no idea why it started to appear now)
Handle the case where the required TLS buffer increases after the connection has been initiated.
Modified:
tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java?rev=1727992&r1=1727991&r2=1727992&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java Mon Feb 1 19:47:13 2016
@@ -558,18 +558,33 @@ public class SecureNioChannel extends Ni
if (unwrap.getStatus() == Status.BUFFER_UNDERFLOW) {
break;
}
- } else if (unwrap.getStatus() == Status.BUFFER_OVERFLOW && read > 0) {
- //buffer overflow can happen, if we have read data, then
- //empty out the dst buffer before we do another read
- break;
+ } else if (unwrap.getStatus() == Status.BUFFER_OVERFLOW) {
+ if (read > 0) {
+ // Buffer overflow can happen if we have read data. Return
+ // so the destination buffer can be emptied before another
+ // read is attempted
+ break;
+ } else {
+ // The SSL session has increased the required buffer size
+ // since the buffer was created.
+ if (dst == socket.getSocketBufferHandler().getReadBuffer()) {
+ // This is the normal case for this code
+ socket.getSocketBufferHandler().expand(
+ sslEngine.getSession().getApplicationBufferSize());
+ dst = socket.getSocketBufferHandler().getReadBuffer();
+ } else {
+ // Can't expand the buffer as there is no way to signal
+ // to the caller that the buffer has been replaced.
+ throw new IOException(
+ sm.getString("channel.nio.ssl.unwrapFail", unwrap.getStatus()));
+ }
+ }
} else {
- //here we should trap BUFFER_OVERFLOW and call expand on the buffer
- //for now, throw an exception, as we initialized the buffers
- //in the constructor
+ // Something else went wrong
throw new IOException(sm.getString("channel.nio.ssl.unwrapFail", unwrap.getStatus()));
}
- } while ( (netInBuffer.position() != 0)); //continue to unwrapping as long as the input buffer has stuff
- return (read);
+ } while (netInBuffer.position() != 0); //continue to unwrapping as long as the input buffer has stuff
+ return read;
}
/**
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1727992&r1=1727991&r2=1727992&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Mon Feb 1 19:47:13 2016
@@ -99,6 +99,10 @@
New configuration option <code>ajpFlush</code> for the AJP connectors
to disable the sending of AJP flush packets. (rjung)
</add>
+ <fix>
+ Handle the case in the NIO connector where the required TLS buffer sizes
+ increase after the connection has been initiated. (markt)
+ </fix>
</changelog>
</subsection>
</section>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1727992 - in /tomcat/trunk: java/org/apache/tomcat/util/net/SecureNioChannel.java
webapps/docs/changelog.xml
Posted by Rémy Maucherat <re...@apache.org>.
2016-02-02 10:46 GMT+01:00 Mark Thomas <ma...@apache.org>:
> > And the fields from Record are static (obviously) and final. The value
> > returned thus shouldn't be able to change.
>
> But acceptLargeFragments can change via a call to
> SSLSessionImpl.expandBufferSizes().
>
> Correct, thanks, the code has the full explanation actually.
acceptLargeFragments could default to true, which will then cause
getApplicationBufferSize to have the compatible non compliant value:
/**
* Use large packet sizes now or follow RFC 2246 packet sizes (2^14)
* until changed.
*
* In the TLS specification (section 6.2.1, RFC2246), it is not
* recommended that the plaintext has more than 2^14 bytes.
* However, some TLS implementations violate the specification.
* This is a workaround for interoperability with these stacks.
*
* Application could accept large fragments up to 2^15 bytes by
* setting the system property jsse.SSLEngine.acceptLargeFragments
* to "true".
*/
private boolean acceptLargeFragments =
Debug.getBooleanProperty("jsse.SSLEngine.acceptLargeFragments",
false);
I don't plan to port the fix to NIO2 at the moment, since it is a
compatibility flag that can be adjusted by users and it would remove my
buffer flexibility :(
Rémy
Re: svn commit: r1727992 - in /tomcat/trunk:
java/org/apache/tomcat/util/net/SecureNioChannel.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 02/02/2016 09:29, Rémy Maucherat wrote:
> 2016-02-02 1:01 GMT+01:00 Mark Thomas <ma...@apache.org>:
>
>>> Well, the design is so wrong.
>>
>> I'm not a fan of the solution either but I couldn't see a better way to
>> resize the buffer.
>>
>> I thought about:
>> - custom exception - rejected since exceptions are slow and flow control
>> via exception is bad
>> - custom return value (-2, Integer.MIN_VALUE or similar) - rejected
>> because it is non-standard and would require changes up the call-stack
>> to handle it.
>> - increasing the default buffer size - rejected as the smaller buffer
>> is enough in nearly all cases
>>
>> Anything else I thought of required invasive API changes. A related
>> issue is the read(ByteBuffer) provides no way to expose the expanded
>> ByteBuffer to the caller but that method is part of the ByteChannel API.
>>
>> Suggestions welcome.
>>
>
> Will think about that :)
> At some point I'll likely have to add another read buffer in
> SecureNio2Channel since I would like more flexibility ...
>
>>
>>> BTW, what is the
>>> getSession().getApplicationBufferSize() value here ? And that's with
>>> OpenSSL or JSSE ?
>>
>> Roughly 16k or 32k for JSSE with current Oracle Java 8 as far as I can
>> tell. Something, I didn't figure out what, was triggering a switch to
>> the larger buffer size after we had initialised the buffers.
>>
>> The OpenSSL implementation only ever uses 16k so it shouldn't hit this
>> code.
>>
> IMO there's something wrong with the behavior since my JDK 8 source is
> (for JSSE):
>
> @Override
> public synchronized int getPacketBufferSize() {
> return acceptLargeFragments ?
> Record.maxLargeRecordSize : Record.maxRecordSize;
> }
>
> @Override
> public synchronized int getApplicationBufferSize() {
> return getPacketBufferSize() - Record.headerSize;
> }
>
> And the fields from Record are static (obviously) and final. The value
> returned thus shouldn't be able to change.
But acceptLargeFragments can change via a call to
SSLSessionImpl.expandBufferSizes().
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1727992 - in /tomcat/trunk: java/org/apache/tomcat/util/net/SecureNioChannel.java
webapps/docs/changelog.xml
Posted by Rémy Maucherat <re...@apache.org>.
2016-02-02 1:01 GMT+01:00 Mark Thomas <ma...@apache.org>:
> > Well, the design is so wrong.
>
> I'm not a fan of the solution either but I couldn't see a better way to
> resize the buffer.
>
> I thought about:
> - custom exception - rejected since exceptions are slow and flow control
> via exception is bad
> - custom return value (-2, Integer.MIN_VALUE or similar) - rejected
> because it is non-standard and would require changes up the call-stack
> to handle it.
> - increasing the default buffer size - rejected as the smaller buffer
> is enough in nearly all cases
>
> Anything else I thought of required invasive API changes. A related
> issue is the read(ByteBuffer) provides no way to expose the expanded
> ByteBuffer to the caller but that method is part of the ByteChannel API.
>
> Suggestions welcome.
>
Will think about that :)
At some point I'll likely have to add another read buffer in
SecureNio2Channel since I would like more flexibility ...
>
> > BTW, what is the
> > getSession().getApplicationBufferSize() value here ? And that's with
> > OpenSSL or JSSE ?
>
> Roughly 16k or 32k for JSSE with current Oracle Java 8 as far as I can
> tell. Something, I didn't figure out what, was triggering a switch to
> the larger buffer size after we had initialised the buffers.
>
> The OpenSSL implementation only ever uses 16k so it shouldn't hit this
> code.
>
> IMO there's something wrong with the behavior since my JDK 8 source is
(for JSSE):
@Override
public synchronized int getPacketBufferSize() {
return acceptLargeFragments ?
Record.maxLargeRecordSize : Record.maxRecordSize;
}
@Override
public synchronized int getApplicationBufferSize() {
return getPacketBufferSize() - Record.headerSize;
}
And the fields from Record are static (obviously) and final. The value
returned thus shouldn't be able to change.
Rémy
Re: svn commit: r1727992 - in /tomcat/trunk:
java/org/apache/tomcat/util/net/SecureNioChannel.java
webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 01/02/2016 23:46, Rémy Maucherat wrote:
> 2016-02-01 20:47 GMT+01:00 <ma...@apache.org>:
>
>> Author: markt
>> Date: Mon Feb 1 19:47:13 2016
>> New Revision: 1727992
>>
>> URL: http://svn.apache.org/viewvc?rev=1727992&view=rev
>> Log:
>> Fix a consistent unit test failure on OSX (no idea why it started to
>> appear now)
>> Handle the case where the required TLS buffer increases after the
>> connection has been initiated.
>>
>
> Well, the design is so wrong.
I'm not a fan of the solution either but I couldn't see a better way to
resize the buffer.
I thought about:
- custom exception - rejected since exceptions are slow and flow control
via exception is bad
- custom return value (-2, Integer.MIN_VALUE or similar) - rejected
because it is non-standard and would require changes up the call-stack
to handle it.
- increasing the default buffer size - rejected as the smaller buffer
is enough in nearly all cases
Anything else I thought of required invasive API changes. A related
issue is the read(ByteBuffer) provides no way to expose the expanded
ByteBuffer to the caller but that method is part of the ByteChannel API.
Suggestions welcome.
> BTW, what is the
> getSession().getApplicationBufferSize() value here ? And that's with
> OpenSSL or JSSE ?
Roughly 16k or 32k for JSSE with current Oracle Java 8 as far as I can
tell. Something, I didn't figure out what, was triggering a switch to
the larger buffer size after we had initialised the buffers.
The OpenSSL implementation only ever uses 16k so it shouldn't hit this code.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1727992 - in /tomcat/trunk: java/org/apache/tomcat/util/net/SecureNioChannel.java
webapps/docs/changelog.xml
Posted by Rémy Maucherat <re...@apache.org>.
2016-02-01 20:47 GMT+01:00 <ma...@apache.org>:
> Author: markt
> Date: Mon Feb 1 19:47:13 2016
> New Revision: 1727992
>
> URL: http://svn.apache.org/viewvc?rev=1727992&view=rev
> Log:
> Fix a consistent unit test failure on OSX (no idea why it started to
> appear now)
> Handle the case where the required TLS buffer increases after the
> connection has been initiated.
>
Well, the design is so wrong. BTW, what is the
getSession().getApplicationBufferSize() value here ? And that's with
OpenSSL or JSSE ?
Rémy
>
> Modified:
> tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java
> tomcat/trunk/webapps/docs/changelog.xml
>
> Modified:
> tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java?rev=1727992&r1=1727991&r2=1727992&view=diff
>
> ==============================================================================
> --- tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java
> (original)
> +++ tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java Mon
> Feb 1 19:47:13 2016
> @@ -558,18 +558,33 @@ public class SecureNioChannel extends Ni
> if (unwrap.getStatus() == Status.BUFFER_UNDERFLOW) {
> break;
> }
> - } else if (unwrap.getStatus() == Status.BUFFER_OVERFLOW &&
> read > 0) {
> - //buffer overflow can happen, if we have read data, then
> - //empty out the dst buffer before we do another read
> - break;
> + } else if (unwrap.getStatus() == Status.BUFFER_OVERFLOW) {
> + if (read > 0) {
> + // Buffer overflow can happen if we have read data.
> Return
> + // so the destination buffer can be emptied before
> another
> + // read is attempted
> + break;
> + } else {
> + // The SSL session has increased the required buffer
> size
> + // since the buffer was created.
> + if (dst ==
> socket.getSocketBufferHandler().getReadBuffer()) {
> + // This is the normal case for this code
> + socket.getSocketBufferHandler().expand(
> +
> sslEngine.getSession().getApplicationBufferSize());
> + dst =
> socket.getSocketBufferHandler().getReadBuffer();
> + } else {
> + // Can't expand the buffer as there is no way to
> signal
> + // to the caller that the buffer has been
> replaced.
> + throw new IOException(
> +
> sm.getString("channel.nio.ssl.unwrapFail", unwrap.getStatus()));
> + }
> + }
> } else {
> - //here we should trap BUFFER_OVERFLOW and call expand on
> the buffer
> - //for now, throw an exception, as we initialized the
> buffers
> - //in the constructor
> + // Something else went wrong
> throw new
> IOException(sm.getString("channel.nio.ssl.unwrapFail", unwrap.getStatus()));
> }
> - } while ( (netInBuffer.position() != 0)); //continue to
> unwrapping as long as the input buffer has stuff
> - return (read);
> + } while (netInBuffer.position() != 0); //continue to unwrapping
> as long as the input buffer has stuff
> + return read;
> }
>
> /**
>
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL:
> http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1727992&r1=1727991&r2=1727992&view=diff
>
> ==============================================================================
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Mon Feb 1 19:47:13 2016
> @@ -99,6 +99,10 @@
> New configuration option <code>ajpFlush</code> for the AJP
> connectors
> to disable the sending of AJP flush packets. (rjung)
> </add>
> + <fix>
> + Handle the case in the NIO connector where the required TLS
> buffer sizes
> + increase after the connection has been initiated. (markt)
> + </fix>
> </changelog>
> </subsection>
> </section>
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>