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 2012/08/30 23:57:16 UTC
svn commit: r1379178 - in /tomcat/trunk: java/org/apache/coyote/http11/
test/org/apache/coyote/http11/
Author: markt
Date: Thu Aug 30 21:57:15 2012
New Revision: 1379178
URL: http://svn.apache.org/viewvc?rev=1379178&view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53677
Ensure a 500 response of the HTTP headers exceed the size limit
Added:
tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java (with props)
Modified:
tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java
tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
Modified: tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java?rev=1379178&r1=1379177&r2=1379178&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java Thu Aug 30 21:57:15 2012
@@ -761,12 +761,21 @@ public abstract class AbstractHttp11Proc
// Validate and write response headers
try {
- prepareResponse();
+// try {
+ prepareResponse();
+// } catch (IllegalStateException e) {
+ // Headers too big. Likely too late to do anything about it
+// response.reset();
+// response.setStatus(500);
+// response.setHeader("Connection", "close");
+// response.sendHeaders();
+// }
getOutputBuffer().commit();
} catch (IOException e) {
// Set error flag
error = true;
}
+
} else if (actionCode == ActionCode.ACK) {
// Acknowledge request
// Send a 100 status back if it makes sense (response not committed
@@ -1009,6 +1018,15 @@ public abstract class AbstractHttp11Proc
setCometTimeouts(socketWrapper);
} catch (InterruptedIOException e) {
error = true;
+ } catch (HeadersTooLargeException e) {
+ error = true;
+ // The response should not have been committed but check it
+ // anyway to be safe
+ if (!response.isCommitted()) {
+ response.reset();
+ response.setStatus(500);
+ response.setHeader("Connection", "close");
+ }
} catch (Throwable t) {
ExceptionUtils.handleThrowable(t);
getLog().error(sm.getString(
Modified: tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java?rev=1379178&r1=1379177&r2=1379178&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java (original)
+++ tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java Thu Aug 30 21:57:15 2012
@@ -250,7 +250,10 @@ public abstract class AbstractOutputBuff
// Recycle Request object
response.recycle();
-
+ // These will need to be reset if the reset was triggered by the error
+ // handling if the headers were too large
+ pos = 0;
+ byteCount = 0;
}
/**
@@ -538,10 +541,9 @@ public abstract class AbstractOutputBuff
* Checks to see if there is enough space in the buffer to write the
* requested number of bytes.
*/
- private void checkLengthBeforeWrite(int length)
- throws IllegalStateException {
+ private void checkLengthBeforeWrite(int length) {
if (pos + length > buf.length) {
- throw new IllegalStateException(
+ throw new HeadersTooLargeException(
sm.getString("iob.responseheadertoolarge.error"));
}
}
Added: tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java?rev=1379178&view=auto
==============================================================================
--- tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java (added)
+++ tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java Thu Aug 30 21:57:15 2012
@@ -0,0 +1,42 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.coyote.http11;
+
+/**
+ * Exception used to mark the specific error condition of the HTTP headers
+ * exceeding the maximum permitted size.
+ */
+public class HeadersTooLargeException extends IllegalStateException {
+
+ private static final long serialVersionUID = 1L;
+
+ public HeadersTooLargeException() {
+ super();
+ }
+
+ public HeadersTooLargeException(String message, Throwable cause) {
+ super(message, cause);
+ }
+
+ public HeadersTooLargeException(String s) {
+ super(s);
+ }
+
+ public HeadersTooLargeException(Throwable cause) {
+ super(cause);
+ }
+}
Propchange: tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java
------------------------------------------------------------------------------
svn:eol-style = native
Modified: tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java?rev=1379178&r1=1379177&r2=1379178&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java (original)
+++ tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java Thu Aug 30 21:57:15 2012
@@ -18,6 +18,7 @@ package org.apache.coyote.http11;
import java.io.File;
import java.io.IOException;
+import java.nio.CharBuffer;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
@@ -309,6 +310,67 @@ public class TestAbstractHttp11Processor
assertEquals("OK", responseBody.toString());
}
+ @Test
+ public void testBug53677a() throws Exception {
+ doTestBug53677(false);
+ }
+
+ @Test
+ public void testBug53677b() throws Exception {
+ doTestBug53677(true);
+ }
+
+ private void doTestBug53677(boolean flush) throws Exception {
+ Tomcat tomcat = getTomcatInstance();
+
+ // Must have a real docBase - just use temp
+ Context ctxt = tomcat.addContext("",
+ System.getProperty("java.io.tmpdir"));
+
+ Tomcat.addServlet(ctxt, "LargeHeaderServlet",
+ new LargeHeaderServlet(flush));
+ ctxt.addServletMapping("/test", "LargeHeaderServlet");
+
+ tomcat.start();
+
+ ByteChunk responseBody = new ByteChunk();
+ Map<String,List<String>> responseHeaders = new HashMap<>();
+ int rc = getUrl("http://localhost:" + getPort() + "/test", responseBody,
+ responseHeaders);
+
+ assertEquals(HttpServletResponse.SC_INTERNAL_SERVER_ERROR, rc);
+ if (responseBody.getLength() > 0) {
+ // It will be >0 if the standard error page handlign has been
+ // triggered
+ assertFalse(responseBody.toString().contains("FAIL"));
+ }
+ }
+
+ private static final class LargeHeaderServlet extends HttpServlet {
+
+ private static final long serialVersionUID = 1L;
+
+ boolean flush = false;
+
+ public LargeHeaderServlet(boolean flush) {
+ this.flush = flush;
+ }
+
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse resp)
+ throws ServletException, IOException {
+ String largeValue =
+ CharBuffer.allocate(10000).toString().replace('\0', 'x');
+ resp.setHeader("x-Test", largeValue);
+ if (flush) {
+ resp.flushBuffer();
+ }
+ resp.setContentType("text/plain");
+ resp.getWriter().print("FAIL");
+ }
+
+ }
+
// flushes with no content-length set
// should result in chunking on HTTP 1.1
private static final class NoContentLengthFlushingServlet
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1379178 - in /tomcat/trunk: java/org/apache/coyote/http11/
test/org/apache/coyote/http11/
Posted by Konstantin Kolinko <kn...@gmail.com>.
2012/8/31 Konstantin Kolinko <kn...@gmail.com>:
> 2012/8/31 <ma...@apache.org>:
>> Author: markt
>> Date: Thu Aug 30 21:57:15 2012
>> New Revision: 1379178
>>
>> URL: http://svn.apache.org/viewvc?rev=1379178&view=rev
>> Log:
>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53677
>> Ensure a 500 response of the HTTP headers exceed the size limit
>>
>
>> --- tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java (original)
>> +++ tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java Thu Aug 30 21:57:15 2012
>> @@ -250,7 +250,10 @@ public abstract class AbstractOutputBuff
>>
>> // Recycle Request object
>> response.recycle();
>> -
>> + // These will need to be reset if the reset was triggered by the error
>> + // handling if the headers were too large
>> + pos = 0;
>> + byteCount = 0;
>> }
>
> There is a use case when there is custom error page for 401 response.
> In that case authentication headers are set before custom error page
> is requested and have to be preserved. (BZ 42409)
>
> I have yet to check it, but if that processing is broken by this, I will be -1.
OK. I tested and there are no regressions from this.
The manager webapp still works (it uses a custom error 401 page), and
code reproducing BZ 42409 still works.
For record, here it is the difference between
1) org.apache.coyote.http11.AbstractOutputBuffer#reset()
Called from
<- org.apache.coyote.Response#reset()
which does "headers.clear()" among other things
<- called by org.apache.catalina.connector.Response#reset()
<- implements javax.servlet.SevletResponse#reset()
which clears all, including the the headers,
and
2) org.apache.catalina.connector.OutputBuffer#reset()
<- called by org.apache.catalina.connector.Response#resetBuffer()
<- implements javax.servlet.SevletResponse#resetBuffer()
which clears the buffer only, leaving the headers and status code intact.
:)
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1379178 - in /tomcat/trunk: java/org/apache/coyote/http11/
test/org/apache/coyote/http11/
Posted by Konstantin Kolinko <kn...@gmail.com>.
2012/8/31 <ma...@apache.org>:
> Author: markt
> Date: Thu Aug 30 21:57:15 2012
> New Revision: 1379178
>
> URL: http://svn.apache.org/viewvc?rev=1379178&view=rev
> Log:
> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53677
> Ensure a 500 response of the HTTP headers exceed the size limit
>
> --- tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java (original)
> +++ tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java Thu Aug 30 21:57:15 2012
> @@ -250,7 +250,10 @@ public abstract class AbstractOutputBuff
>
> // Recycle Request object
> response.recycle();
> -
> + // These will need to be reset if the reset was triggered by the error
> + // handling if the headers were too large
> + pos = 0;
> + byteCount = 0;
> }
There is a use case when there is custom error page for 401 response.
In that case authentication headers are set before custom error page
is requested and have to be preserved. (BZ 42409)
I have yet to check it, but if that processing is broken by this, I will be -1.
> --- tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java (added)
> +++ tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java Thu Aug 30 21:57:15 2012
> @@ -0,0 +1,42 @@
> +/*
> + * Licensed to the Apache Software Foundation (ASF) under one or more
> + * contributor license agreements. See the NOTICE file distributed with
> + * this work for additional information regarding copyright ownership.
> + * The ASF licenses this file to You under the Apache License, Version 2.0
> + * (the "License"); you may not use this file except in compliance with
> + * the License. You may obtain a copy of the License at
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +package org.apache.coyote.http11;
> +
> +/**
> + * Exception used to mark the specific error condition of the HTTP headers
> + * exceeding the maximum permitted size.
> + */
> +public class HeadersTooLargeException extends IllegalStateException {
> +
> + private static final long serialVersionUID = 1L;
> +
> + public HeadersTooLargeException() {
> + super();
> + }
> +
> + public HeadersTooLargeException(String message, Throwable cause) {
> + super(message, cause);
> + }
> +
> + public HeadersTooLargeException(String s) {
> + super(s);
> + }
> +
> + public HeadersTooLargeException(Throwable cause) {
> + super(cause);
> + }
I wonder whether UCDetector will remove some of the above.
Just joking.
If it is supposed to be used by 3rd parties, it is better to keep all
4 constructors.
> +}
>
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1379178 - in /tomcat/trunk: java/org/apache/coyote/http11/
test/org/apache/coyote/http11/
Posted by Mark Thomas <ma...@apache.org>.
On 31/08/2012 11:29, Rainer Jung wrote:
> On 30.08.2012 23:57, markt@apache.org wrote:
>> Author: markt
>> Date: Thu Aug 30 21:57:15 2012
>> New Revision: 1379178
>>
>> URL: http://svn.apache.org/viewvc?rev=1379178&view=rev
>> Log:
>> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53677
>> Ensure a 500 response of the HTTP headers exceed the size limit
>>
>> Added:
>>
>> tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java
>> (with props)
>> Modified:
>>
>> tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
>> tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java
>>
>> tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
>>
>>
>> Modified:
>> tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
>> URL:
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java?rev=1379178&r1=1379177&r2=1379178&view=diff
>>
>> ==============================================================================
>>
>> ---
>> tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java (original)
>>
>> +++
>> tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java Thu
>> Aug 30 21:57:15 2012
>> @@ -761,12 +761,21 @@ public abstract class AbstractHttp11Proc
>>
>> // Validate and write response headers
>> try {
>> - prepareResponse();
>> +// try {
>> + prepareResponse();
>> +// } catch (IllegalStateException e) {
>> + // Headers too big. Likely too late to do
>> anything about it
>> +// response.reset();
>> +// response.setStatus(500);
>> +// response.setHeader("Connection", "close");
>> +// response.sendHeaders();
>> +// }
>
> Not sure but the above commented out code looks like a leftover from an
> earlier attempt, before the code was latter moved further down. If I'm
> right you might want to remove the above change. At least it would fit
> to your prefered slogan "Remove unused code" :)
Yes. It was a left over. I'll remove it. Thanks for the catch.
> Same for TC 7.
Ack.
Makr
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1379178 - in /tomcat/trunk: java/org/apache/coyote/http11/
test/org/apache/coyote/http11/
Posted by Rainer Jung <ra...@kippdata.de>.
On 30.08.2012 23:57, markt@apache.org wrote:
> Author: markt
> Date: Thu Aug 30 21:57:15 2012
> New Revision: 1379178
>
> URL: http://svn.apache.org/viewvc?rev=1379178&view=rev
> Log:
> Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=53677
> Ensure a 500 response of the HTTP headers exceed the size limit
>
> Added:
> tomcat/trunk/java/org/apache/coyote/http11/HeadersTooLargeException.java (with props)
> Modified:
> tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
> tomcat/trunk/java/org/apache/coyote/http11/AbstractOutputBuffer.java
> tomcat/trunk/test/org/apache/coyote/http11/TestAbstractHttp11Processor.java
>
> Modified: tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java?rev=1379178&r1=1379177&r2=1379178&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java (original)
> +++ tomcat/trunk/java/org/apache/coyote/http11/AbstractHttp11Processor.java Thu Aug 30 21:57:15 2012
> @@ -761,12 +761,21 @@ public abstract class AbstractHttp11Proc
>
> // Validate and write response headers
> try {
> - prepareResponse();
> +// try {
> + prepareResponse();
> +// } catch (IllegalStateException e) {
> + // Headers too big. Likely too late to do anything about it
> +// response.reset();
> +// response.setStatus(500);
> +// response.setHeader("Connection", "close");
> +// response.sendHeaders();
> +// }
Not sure but the above commented out code looks like a leftover from an
earlier attempt, before the code was latter moved further down. If I'm
right you might want to remove the above change. At least it would fit
to your prefered slogan "Remove unused code" :)
Same for TC 7.
> getOutputBuffer().commit();
> } catch (IOException e) {
> // Set error flag
> error = true;
> }
> +
> } else if (actionCode == ActionCode.ACK) {
> // Acknowledge request
> // Send a 100 status back if it makes sense (response not committed
> @@ -1009,6 +1018,15 @@ public abstract class AbstractHttp11Proc
> setCometTimeouts(socketWrapper);
> } catch (InterruptedIOException e) {
> error = true;
> + } catch (HeadersTooLargeException e) {
> + error = true;
> + // The response should not have been committed but check it
> + // anyway to be safe
> + if (!response.isCommitted()) {
> + response.reset();
> + response.setStatus(500);
> + response.setHeader("Connection", "close");
> + }
> } catch (Throwable t) {
> ExceptionUtils.handleThrowable(t);
> getLog().error(sm.getString(
Regards,
Rainer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org