Re: [tomcat] branch main updated: Add expanded unit tests for HEAD and fix bugs identified in buffering

2021-06-16 Thread Rémy Maucherat
On Wed, Jun 16, 2021 at 11:15 AM Mark Thomas  wrote:

> On 16/06/2021 10:11, ma...@apache.org wrote:
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > markt pushed a commit to branch main
> > in repository https://gitbox.apache.org/repos/asf/tomcat.git
> >
> >
> > The following commit(s) were added to refs/heads/main by this push:
> >   new e98409e  Add expanded unit tests for HEAD and fix bugs
> identified in buffering
> > e98409e is described below
> >
> > commit e98409e3bb9ae1e29e607ca511eb99dfcf3014a0
> > Author: Mark Thomas 
> > AuthorDate: Thu Jun 3 16:44:41 2021 +0100
> >
> >  Add expanded unit tests for HEAD and fix bugs identified in
> buffering
>
> Please try and break this. I checked and re-checked this and I think it
> is correct but there is an opportunity here for an off-by-one error
> triggering response corruption and that would be really bad.
>

This looks good to me, as demonstrated by your test failure, you should now
get a more "perfect" buffering behavior.

Rémy


>
> Thanks,
>
> Mark
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
>


Re: [tomcat] branch main updated: Add expanded unit tests for HEAD and fix bugs identified in buffering

2021-06-16 Thread Mark Thomas

On 16/06/2021 10:11, ma...@apache.org wrote:

This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
  new e98409e  Add expanded unit tests for HEAD and fix bugs identified in 
buffering
e98409e is described below

commit e98409e3bb9ae1e29e607ca511eb99dfcf3014a0
Author: Mark Thomas 
AuthorDate: Thu Jun 3 16:44:41 2021 +0100

 Add expanded unit tests for HEAD and fix bugs identified in buffering


Please try and break this. I checked and re-checked this and I think it 
is correct but there is an opportunity here for an off-by-one error 
triggering response corruption and that would be really bad.


Thanks,

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



[tomcat] branch main updated: Add expanded unit tests for HEAD and fix bugs identified in buffering

2021-06-16 Thread markt
This is an automated email from the ASF dual-hosted git repository.

markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git


The following commit(s) were added to refs/heads/main by this push:
 new e98409e  Add expanded unit tests for HEAD and fix bugs identified in 
buffering
e98409e is described below

commit e98409e3bb9ae1e29e607ca511eb99dfcf3014a0
Author: Mark Thomas 
AuthorDate: Thu Jun 3 16:44:41 2021 +0100

Add expanded unit tests for HEAD and fix bugs identified in buffering
---
 .../apache/catalina/connector/OutputBuffer.java|  10 +-
 .../servlet/http/TestHttpServletDoHead.java| 223 +
 webapps/docs/changelog.xml |   8 +
 3 files changed, 236 insertions(+), 5 deletions(-)

diff --git a/java/org/apache/catalina/connector/OutputBuffer.java 
b/java/org/apache/catalina/connector/OutputBuffer.java
index 0ea4dbd..d7a6783 100644
--- a/java/org/apache/catalina/connector/OutputBuffer.java
+++ b/java/org/apache/catalina/connector/OutputBuffer.java
@@ -527,7 +527,7 @@ public class OutputBuffer extends Writer {
 while (sOff < sEnd) {
 int n = transfer(s, sOff, sEnd - sOff, cb);
 sOff += n;
-if (isFull(cb)) {
+if (sOff < sEnd && isFull(cb)) {
 flushCharBuffer();
 }
 }
@@ -684,7 +684,7 @@ public class OutputBuffer extends Writer {
 int n = transfer(src, off, len, bb);
 len = len - n;
 off = off + n;
-if (isFull(bb)) {
+if (len > 0 && isFull(bb)) {
 flushByteBuffer();
 appendByteArray(src, off, len);
 }
@@ -736,7 +736,7 @@ public class OutputBuffer extends Writer {
 appendByteBuffer(from);
 } else {
 transfer(from, bb);
-if (isFull(bb)) {
+if (from.hasRemaining() && isFull(bb)) {
 flushByteBuffer();
 appendByteBuffer(from);
 }
@@ -749,7 +749,7 @@ public class OutputBuffer extends Writer {
 }
 
 int limit = bb.capacity();
-while (len >= limit) {
+while (len > limit) {
 realWriteBytes(ByteBuffer.wrap(src, off, limit));
 len = len - limit;
 off = off + limit;
@@ -767,7 +767,7 @@ public class OutputBuffer extends Writer {
 
 int limit = bb.capacity();
 int fromLimit = from.limit();
-while (from.remaining() >= limit) {
+while (from.remaining() > limit) {
 from.limit(from.position() + limit);
 realWriteBytes(from.slice());
 from.position(from.limit());
diff --git a/test/jakarta/servlet/http/TestHttpServletDoHead.java 
b/test/jakarta/servlet/http/TestHttpServletDoHead.java
new file mode 100644
index 000..006c560
--- /dev/null
+++ b/test/jakarta/servlet/http/TestHttpServletDoHead.java
@@ -0,0 +1,223 @@
+/*
+ * 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 jakarta.servlet.http;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.io.PrintWriter;
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import jakarta.servlet.ServletException;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.Parameterized;
+import org.junit.runners.Parameterized.Parameter;
+
+import org.apache.catalina.core.StandardContext;
+import org.apache.catalina.startup.Tomcat;
+import org.apache.catalina.startup.TomcatBaseTest;
+import org.apache.tomcat.util.buf.ByteChunk;
+import org.apache.tomcat.util.collections.CaseInsensitiveKeyMap;
+
+@RunWith(Parameterized.class)
+public class TestHttpServletDoHead extends TomcatBaseTest {
+
+// Tomcat has a minimum output buffer size of 8 * 1024.
+// (8 * 1024) /16 = 512
+
+private static final String VALID = "** valid data **";
+private static final String INVALID = "* invalid data *";
+
+private static final Integer BUFFERS[] = new Integer[] { Integer.valueOf 
(16), Integer.valueOf(8 * 1024),