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 2023/01/05 15:16:40 UTC
[tomcat] 01/03: Refactor MessageBytes make conversions consistent with most recent set
This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch 8.5.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 8e40ef3ae8d6f27324cc6370f63809a3d1870f8f
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Sep 1 14:05:20 2022 +0100
Refactor MessageBytes make conversions consistent with most recent set
This is preparation for fixing BZ 66196
https://bz.apache.org/bugzilla/show_bug.cgi?id=66196
---
.../apache/catalina/core/ApplicationContext.java | 1 +
java/org/apache/catalina/mapper/Mapper.java | 1 +
.../catalina/valves/rewrite/RewriteValve.java | 18 +-
java/org/apache/tomcat/util/buf/MessageBytes.java | 105 ++++++-----
.../util/buf/TestMessageBytesConversion.java | 207 +++++++++++++++++++++
5 files changed, 267 insertions(+), 65 deletions(-)
diff --git a/java/org/apache/catalina/core/ApplicationContext.java b/java/org/apache/catalina/core/ApplicationContext.java
index b3dbbbd3db..ab125aa829 100644
--- a/java/org/apache/catalina/core/ApplicationContext.java
+++ b/java/org/apache/catalina/core/ApplicationContext.java
@@ -465,6 +465,7 @@ public class ApplicationContext implements ServletContext {
try {
// Map the URI
+ uriMB.setChars(MessageBytes.EMPTY_CHAR_ARRAY, 0, 0);
CharChunk uriCC = uriMB.getCharChunk();
try {
uriCC.append(context.getPath());
diff --git a/java/org/apache/catalina/mapper/Mapper.java b/java/org/apache/catalina/mapper/Mapper.java
index eb1222d876..d160d9343e 100644
--- a/java/org/apache/catalina/mapper/Mapper.java
+++ b/java/org/apache/catalina/mapper/Mapper.java
@@ -695,6 +695,7 @@ public final class Mapper {
if (defaultHostName == null) {
return;
}
+ host.setChars(MessageBytes.EMPTY_CHAR_ARRAY, 0, 0);
host.getCharChunk().append(defaultHostName);
}
host.toChars();
diff --git a/java/org/apache/catalina/valves/rewrite/RewriteValve.java b/java/org/apache/catalina/valves/rewrite/RewriteValve.java
index dfad2ad907..ff41651288 100644
--- a/java/org/apache/catalina/valves/rewrite/RewriteValve.java
+++ b/java/org/apache/catalina/valves/rewrite/RewriteValve.java
@@ -508,49 +508,39 @@ public class RewriteValve extends ValveBase {
contextPath = request.getContextPath();
}
// Populated the encoded (i.e. undecoded) requestURI
- request.getCoyoteRequest().requestURI().setString(null);
+ request.getCoyoteRequest().requestURI().setChars(MessageBytes.EMPTY_CHAR_ARRAY, 0, 0);
CharChunk chunk = request.getCoyoteRequest().requestURI().getCharChunk();
- chunk.recycle();
if (context) {
// This is neither decoded nor normalized
chunk.append(contextPath);
}
chunk.append(URLEncoder.DEFAULT.encode(urlStringDecoded, uriCharset));
- request.getCoyoteRequest().requestURI().toChars();
// Decoded and normalized URI
// Rewriting may have denormalized the URL
urlStringDecoded = RequestUtil.normalize(urlStringDecoded);
- request.getCoyoteRequest().decodedURI().setString(null);
+ request.getCoyoteRequest().decodedURI().setChars(MessageBytes.EMPTY_CHAR_ARRAY, 0, 0);
chunk = request.getCoyoteRequest().decodedURI().getCharChunk();
- chunk.recycle();
if (context) {
// This is decoded and normalized
chunk.append(request.getServletContext().getContextPath());
}
chunk.append(urlStringDecoded);
- request.getCoyoteRequest().decodedURI().toChars();
// Set the new Query if there is one
if (queryStringDecoded != null) {
- request.getCoyoteRequest().queryString().setString(null);
+ request.getCoyoteRequest().queryString().setChars(MessageBytes.EMPTY_CHAR_ARRAY, 0, 0);
chunk = request.getCoyoteRequest().queryString().getCharChunk();
- chunk.recycle();
chunk.append(URLEncoder.QUERY.encode(queryStringDecoded, uriCharset));
if (qsa && originalQueryStringEncoded != null &&
originalQueryStringEncoded.length() > 0) {
chunk.append('&');
chunk.append(originalQueryStringEncoded);
}
- if (!chunk.isNull()) {
- request.getCoyoteRequest().queryString().toChars();
- }
}
// Set the new host if it changed
if (!host.equals(request.getServerName())) {
- request.getCoyoteRequest().serverName().setString(null);
+ request.getCoyoteRequest().serverName().setChars(MessageBytes.EMPTY_CHAR_ARRAY, 0, 0);
chunk = request.getCoyoteRequest().serverName().getCharChunk();
- chunk.recycle();
chunk.append(host.toString());
- request.getCoyoteRequest().serverName().toChars();
}
request.getMappingData().recycle();
// Reinvoke the whole request recursively
diff --git a/java/org/apache/tomcat/util/buf/MessageBytes.java b/java/org/apache/tomcat/util/buf/MessageBytes.java
index a24ad820e5..857a8febb6 100644
--- a/java/org/apache/tomcat/util/buf/MessageBytes.java
+++ b/java/org/apache/tomcat/util/buf/MessageBytes.java
@@ -51,6 +51,8 @@ public final class MessageBytes implements Cloneable, Serializable {
was a char[] */
public static final int T_CHARS = 3;
+ public static final char[] EMPTY_CHAR_ARRAY = new char[0];
+
private int hashCode=0;
// did we compute the hashcode ?
private boolean hasHashCode=false;
@@ -61,9 +63,6 @@ public final class MessageBytes implements Cloneable, Serializable {
// String
private String strValue;
- // true if a String value was computed. Probably not needed,
- // strValue!=null is the same
- private boolean hasStrValue=false;
/**
* Creates a new, uninitialized MessageBytes object.
@@ -87,7 +86,7 @@ public final class MessageBytes implements Cloneable, Serializable {
}
public boolean isNull() {
- return byteC.isNull() && charC.isNull() && !hasStrValue;
+ return type == T_NULL;
}
/**
@@ -100,7 +99,6 @@ public final class MessageBytes implements Cloneable, Serializable {
strValue=null;
- hasStrValue=false;
hasHashCode=false;
hasLongValue=false;
}
@@ -116,7 +114,6 @@ public final class MessageBytes implements Cloneable, Serializable {
public void setBytes(byte[] b, int off, int len) {
byteC.setBytes( b, off, len );
type=T_BYTES;
- hasStrValue=false;
hasHashCode=false;
hasLongValue=false;
}
@@ -131,7 +128,6 @@ public final class MessageBytes implements Cloneable, Serializable {
public void setChars( char[] c, int off, int len ) {
charC.setChars( c, off, len );
type=T_CHARS;
- hasStrValue=false;
hasHashCode=false;
hasLongValue=false;
}
@@ -141,15 +137,13 @@ public final class MessageBytes implements Cloneable, Serializable {
* @param s The string
*/
public void setString( String s ) {
- strValue=s;
- hasHashCode=false;
- hasLongValue=false;
+ strValue = s;
+ hasHashCode = false;
+ hasLongValue = false;
if (s == null) {
- hasStrValue=false;
- type=T_NULL;
+ type = T_NULL;
} else {
- hasStrValue=true;
- type=T_STR;
+ type = T_STR;
}
}
@@ -161,21 +155,22 @@ public final class MessageBytes implements Cloneable, Serializable {
*/
@Override
public String toString() {
- if (hasStrValue) {
- return strValue;
- }
-
switch (type) {
- case T_CHARS:
- strValue = charC.toString();
- hasStrValue = true;
- return strValue;
- case T_BYTES:
- strValue = byteC.toString();
- hasStrValue = true;
- return strValue;
+ case T_NULL:
+ case T_STR:
+ // No conversion required
+ break;
+ case T_BYTES:
+ type = T_STR;
+ strValue = byteC.toString();
+ break;
+ case T_CHARS:
+ type = T_STR;
+ strValue = charC.toString();
+ break;
}
- return null;
+
+ return strValue;
}
//----------------------------------------
@@ -232,21 +227,26 @@ public final class MessageBytes implements Cloneable, Serializable {
/**
- * Do a char->byte conversion.
+ * Convert to bytes and fill the ByteChunk with the converted value.
*/
public void toBytes() {
- if (isNull()) {
- return;
- }
- if (!byteC.isNull()) {
- type = T_BYTES;
- return;
+ switch (type) {
+ case T_NULL:
+ byteC.recycle();
+ //$FALL-THROUGH$
+ case T_BYTES:
+ // No conversion required
+ return;
+ case T_CHARS:
+ toString();
+ //$FALL-THROUGH$
+ case T_STR: {
+ type = T_BYTES;
+ Charset charset = byteC.getCharset();
+ ByteBuffer result = charset.encode(strValue);
+ byteC.setBytes(result.array(), result.arrayOffset(), result.limit());
+ }
}
- toString();
- type = T_BYTES;
- Charset charset = byteC.getCharset();
- ByteBuffer result = charset.encode(strValue);
- byteC.setBytes(result.array(), result.arrayOffset(), result.limit());
}
@@ -255,18 +255,22 @@ public final class MessageBytes implements Cloneable, Serializable {
* XXX Not optimized - it converts to String first.
*/
public void toChars() {
- if (isNull()) {
- return;
- }
- if (!charC.isNull()) {
- type = T_CHARS;
- return;
+ switch (type) {
+ case T_NULL:
+ charC.recycle();
+ //$FALL-THROUGH$
+ case T_CHARS:
+ // No conversion required
+ return;
+ case T_BYTES:
+ toString();
+ //$FALL-THROUGH$
+ case T_STR: {
+ type = T_CHARS;
+ char cc[] = strValue.toCharArray();
+ charC.setChars(cc, 0, cc.length);
+ }
}
- // inefficient
- toString();
- type = T_CHARS;
- char cc[] = strValue.toCharArray();
- charC.setChars(cc, 0, cc.length);
}
@@ -535,7 +539,6 @@ public final class MessageBytes implements Cloneable, Serializable {
end--;
}
longValue=l;
- hasStrValue=false;
hasHashCode=false;
hasLongValue=true;
type=T_BYTES;
diff --git a/test/org/apache/tomcat/util/buf/TestMessageBytesConversion.java b/test/org/apache/tomcat/util/buf/TestMessageBytesConversion.java
new file mode 100644
index 0000000000..2b0bddd5a5
--- /dev/null
+++ b/test/org/apache/tomcat/util/buf/TestMessageBytesConversion.java
@@ -0,0 +1,207 @@
+/*
+ * 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.tomcat.util.buf;
+
+import java.nio.charset.StandardCharsets;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.function.Consumer;
+
+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.junit.runners.Parameterized.Parameters;
+
+/**
+ * Checks that all MessageBytes getters are consistent with most recently used
+ * setter.
+ */
+@RunWith(Parameterized.class)
+public class TestMessageBytesConversion {
+
+ private static final String PREVIOUS_STRING = "previous-string";
+ private static final byte[] PREVIOUS_BYTES = "previous-bytes".getBytes(StandardCharsets.ISO_8859_1);
+ private static final char[] PREVIOUS_CHARS = "previous-chars".toCharArray();
+
+ private static final String EXPECTED_STRING = "expected";
+ private static final byte[] EXPECTED_BYTES = "expected".getBytes(StandardCharsets.ISO_8859_1);
+ private static final char[] EXPECTED_CHARS = "expected".toCharArray();
+
+ @Parameters(name = "{index}: previous({0}, {1}, {2}, {3}), set {4}, check({5}, {6}, {7}")
+ public static Collection<Object[]> parameters() {
+ List<MessageBytesType> previousTypes = new ArrayList<>();
+ previousTypes.add(MessageBytesType.BYTE);
+ previousTypes.add(MessageBytesType.CHAR);
+ previousTypes.add(MessageBytesType.STRING);
+ previousTypes.add(MessageBytesType.NULL);
+
+ List<MessageBytesType> setTypes = new ArrayList<>();
+ setTypes.add(MessageBytesType.BYTE);
+ setTypes.add(MessageBytesType.CHAR);
+ setTypes.add(MessageBytesType.STRING);
+
+ List<Object[]> parameterSets = new ArrayList<>();
+
+ List<List<MessageBytesType>> previousPermutations = permutations(previousTypes);
+ List<List<MessageBytesType>> checkPermutations = permutations(setTypes);
+
+ for (List<MessageBytesType> setPermutation : previousPermutations) {
+ for (MessageBytesType setType : setTypes) {
+ for (List<MessageBytesType> checkPermutation : checkPermutations) {
+ parameterSets.add(new Object[] {
+ setPermutation.get(0), setPermutation.get(1), setPermutation.get(2), setPermutation.get(3),
+ setType,
+ checkPermutation.get(0), checkPermutation.get(1), checkPermutation.get(2)});
+ }
+ }
+ }
+
+
+ return parameterSets;
+ }
+
+ @Parameter(0)
+ public MessageBytesType setFirst;
+ @Parameter(1)
+ public MessageBytesType setSecond;
+ @Parameter(2)
+ public MessageBytesType setThird;
+ @Parameter(3)
+ public MessageBytesType setFourth;
+
+ @Parameter(4)
+ public MessageBytesType expected;
+
+ @Parameter(5)
+ public MessageBytesType checkFirst;
+ @Parameter(6)
+ public MessageBytesType checkSecond;
+ @Parameter(7)
+ public MessageBytesType checkThird;
+
+
+ @Test
+ public void testConversion() {
+ MessageBytes mb = MessageBytes.newInstance();
+
+ setFirst.setPrevious(mb);
+ setSecond.setPrevious(mb);
+ setThird.setPrevious(mb);
+ setFourth.setPrevious(mb);
+
+ expected.setExpected(mb);
+
+ checkFirst.checkExpected(mb);
+ checkSecond.checkExpected(mb);
+ checkThird.checkExpected(mb);
+ }
+
+
+ @Test
+ public void testConversionNull() {
+ MessageBytes mb = MessageBytes.newInstance();
+
+ setFirst.setPrevious(mb);
+ setSecond.setPrevious(mb);
+ setThird.setPrevious(mb);
+ setFourth.setPrevious(mb);
+
+ mb.setString(null);
+
+ checkFirst.checkNull(mb);
+ checkSecond.checkNull(mb);
+ checkThird.checkNull(mb);
+ }
+
+
+ public static enum MessageBytesType {
+ BYTE((x) -> x.setBytes(PREVIOUS_BYTES, 0, PREVIOUS_BYTES.length),
+ (x) -> x.setBytes(EXPECTED_BYTES, 0, EXPECTED_BYTES.length),
+ (x) -> {x.toBytes(); Assert.assertArrayEquals(EXPECTED_BYTES, x.getByteChunk().getBytes());},
+ (x) -> {x.toBytes(); Assert.assertTrue(x.getByteChunk().isNull());}),
+
+ CHAR((x) -> x.setChars(PREVIOUS_CHARS, 0, PREVIOUS_CHARS.length),
+ (x) -> x.setChars(EXPECTED_CHARS, 0, EXPECTED_CHARS.length),
+ (x) -> {x.toChars(); Assert.assertArrayEquals(EXPECTED_CHARS, x.getCharChunk().getChars());},
+ (x) -> {x.toChars(); Assert.assertTrue(x.getCharChunk().isNull());}),
+
+ STRING((x) -> x.setString(PREVIOUS_STRING),
+ (x) -> x.setString(EXPECTED_STRING),
+ (x) -> Assert.assertEquals(EXPECTED_STRING, x.toString()),
+ (x) -> Assert.assertNull(x.toString())),
+
+ NULL((x) -> x.setString(null),
+ (x) -> x.setString(null),
+ (x) -> Assert.assertTrue(x.isNull()),
+ (x) -> Assert.assertTrue(x.isNull()));
+
+ private final Consumer<MessageBytes> setPrevious;
+ private final Consumer<MessageBytes> setExpected;
+ private final Consumer<MessageBytes> checkExpected;
+ private final Consumer<MessageBytes> checkNull;
+
+ private MessageBytesType(Consumer<MessageBytes> setPrevious, Consumer<MessageBytes> setExpected,
+ Consumer<MessageBytes> checkExpected, Consumer<MessageBytes> checkNull) {
+ this.setPrevious = setPrevious;
+ this.setExpected = setExpected;
+ this.checkExpected = checkExpected;
+ this.checkNull = checkNull;
+ }
+
+ public void setPrevious(MessageBytes mb) {
+ setPrevious.accept(mb);
+ }
+
+ public void setExpected(MessageBytes mb) {
+ setExpected.accept(mb);
+ }
+
+ public void checkExpected(MessageBytes mb) {
+ checkExpected.accept(mb);
+ }
+
+ public void checkNull(MessageBytes mb) {
+ checkNull.accept(mb);
+ }
+ }
+
+
+ private static <T> List<List<T>> permutations(List<T> items) {
+ List<List<T>> results = new ArrayList<>();
+
+ if (items.size() == 1) {
+ results.add(items);
+ } else {
+ List<T> others = new ArrayList<>(items);
+ T first = others.remove(0);
+ List<List<T>> subPermutations = permutations(others);
+
+ for (List<T> subPermutation : subPermutations) {
+ for (int i = 0; i <= subPermutation.size(); i++) {
+ List<T> result = new ArrayList<>(subPermutation);
+ result.add(i, first);
+ results.add(result);
+ }
+ }
+ }
+
+ return results;
+ }
+}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org