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/06/22 18:32:23 UTC

[tomcat] 03/04: First pass at implementing changes for parameter parsing

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

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

commit 61009deff1cf542b6c555121e6af4a6baf7851b6
Author: Mark Thomas <ma...@apache.org>
AuthorDate: Thu Jun 15 12:15:46 2023 +0100

    First pass at implementing changes for parameter parsing
---
 java/org/apache/catalina/connector/Request.java    |  31 +--
 .../apache/catalina/core/StandardHostValve.java    |   3 +-
 .../apache/catalina/core/StandardWrapperValve.java |  18 +-
 .../util/http/InvalidParameterException.java       |  56 +++++
 .../tomcat/util/http/LocalStrings.properties       |   7 +-
 .../tomcat/util/http/ParameterErrorHandling.java   | 100 +++++++++
 java/org/apache/tomcat/util/http/Parameters.java   | 241 ++++++++++-----------
 7 files changed, 313 insertions(+), 143 deletions(-)

diff --git a/java/org/apache/catalina/connector/Request.java b/java/org/apache/catalina/connector/Request.java
index 312b3f4e81..c0afb7553c 100644
--- a/java/org/apache/catalina/connector/Request.java
+++ b/java/org/apache/catalina/connector/Request.java
@@ -103,7 +103,6 @@ import org.apache.tomcat.util.buf.UDecoder;
 import org.apache.tomcat.util.http.CookieProcessor;
 import org.apache.tomcat.util.http.FastHttpDateFormat;
 import org.apache.tomcat.util.http.Parameters;
-import org.apache.tomcat.util.http.Parameters.FailReason;
 import org.apache.tomcat.util.http.Rfc6265CookieProcessor;
 import org.apache.tomcat.util.http.ServerCookie;
 import org.apache.tomcat.util.http.ServerCookies;
@@ -2723,7 +2722,7 @@ public class Request implements HttpServletRequest {
             }
 
             if (!location.isDirectory()) {
-                parameters.setParseFailedReason(FailReason.MULTIPART_CONFIG_INVALID);
+                // TODO parameters.setParseFailedReason(FailReason.MULTIPART_CONFIG_INVALID);
                 partsParseException = new IOException(sm.getString("coyoteRequest.uploadLocationInvalid", location));
                 return;
             }
@@ -2734,7 +2733,7 @@ public class Request implements HttpServletRequest {
             try {
                 factory.setRepository(location.getCanonicalFile());
             } catch (IOException ioe) {
-                parameters.setParseFailedReason(FailReason.IO_ERROR);
+             // TODO parameters.setParseFailedReason(FailReason.IO_ERROR);
                 partsParseException = ioe;
                 return;
             }
@@ -2774,7 +2773,7 @@ public class Request implements HttpServletRequest {
                             // Value separator
                             postSize++;
                             if (postSize > maxPostSize) {
-                                parameters.setParseFailedReason(FailReason.POST_TOO_LARGE);
+                             // TODO parameters.setParseFailedReason(FailReason.POST_TOO_LARGE);
                                 throw new IllegalStateException(sm.getString("coyoteRequest.maxPostSizeExceeded"));
                             }
                         }
@@ -2790,14 +2789,14 @@ public class Request implements HttpServletRequest {
 
                 success = true;
             } catch (InvalidContentTypeException e) {
-                parameters.setParseFailedReason(FailReason.INVALID_CONTENT_TYPE);
+             // TODO parameters.setParseFailedReason(FailReason.INVALID_CONTENT_TYPE);
                 partsParseException = new ServletException(e);
             } catch (SizeException e) {
-                parameters.setParseFailedReason(FailReason.POST_TOO_LARGE);
+             // TODO parameters.setParseFailedReason(FailReason.POST_TOO_LARGE);
                 checkSwallowInput();
                 partsParseException = new IllegalStateException(e);
             } catch (IOException e) {
-                parameters.setParseFailedReason(FailReason.IO_ERROR);
+             // TODO parameters.setParseFailedReason(FailReason.IO_ERROR);
                 partsParseException = new IOException(e);
             } catch (IllegalStateException e) {
                 // addParameters() will set parseFailedReason
@@ -2810,7 +2809,7 @@ public class Request implements HttpServletRequest {
             // be more efficient but it is written this way to be robust with
             // respect to changes in the remainder of the method.
             if (partsParseException != null || !success) {
-                parameters.setParseFailedReason(FailReason.UNKNOWN);
+             // TODO parameters.setParseFailedReason(FailReason.UNKNOWN);
             }
         }
     }
@@ -3102,7 +3101,7 @@ public class Request implements HttpServletRequest {
                         context.getLogger().debug(sm.getString("coyoteRequest.postTooLarge"));
                     }
                     checkSwallowInput();
-                    parameters.setParseFailedReason(FailReason.POST_TOO_LARGE);
+                 // TODO parameters.setParseFailedReason(FailReason.POST_TOO_LARGE);
                     return;
                 }
                 byte[] formData = null;
@@ -3116,7 +3115,7 @@ public class Request implements HttpServletRequest {
                 }
                 try {
                     if (readPostBody(formData, len) != len) {
-                        parameters.setParseFailedReason(FailReason.REQUEST_BODY_INCOMPLETE);
+                     // TODO parameters.setParseFailedReason(FailReason.REQUEST_BODY_INCOMPLETE);
                         return;
                     }
                 } catch (IOException e) {
@@ -3125,7 +3124,7 @@ public class Request implements HttpServletRequest {
                     if (context != null && context.getLogger().isDebugEnabled()) {
                         context.getLogger().debug(sm.getString("coyoteRequest.parseParameters"), e);
                     }
-                    parameters.setParseFailedReason(FailReason.CLIENT_DISCONNECT);
+                 // TODO parameters.setParseFailedReason(FailReason.CLIENT_DISCONNECT);
                     return;
                 }
                 parameters.processParameters(formData, 0, len);
@@ -3135,7 +3134,7 @@ public class Request implements HttpServletRequest {
                     formData = readChunkedPostBody();
                 } catch (IllegalStateException ise) {
                     // chunkedPostTooLarge error
-                    parameters.setParseFailedReason(FailReason.POST_TOO_LARGE);
+                 // TODO parameters.setParseFailedReason(FailReason.POST_TOO_LARGE);
                     Context context = getContext();
                     if (context != null && context.getLogger().isDebugEnabled()) {
                         context.getLogger().debug(sm.getString("coyoteRequest.parseParameters"), ise);
@@ -3143,7 +3142,7 @@ public class Request implements HttpServletRequest {
                     return;
                 } catch (IOException e) {
                     // Client disconnect
-                    parameters.setParseFailedReason(FailReason.CLIENT_DISCONNECT);
+                 // TODO parameters.setParseFailedReason(FailReason.CLIENT_DISCONNECT);
                     Context context = getContext();
                     if (context != null && context.getLogger().isDebugEnabled()) {
                         context.getLogger().debug(sm.getString("coyoteRequest.parseParameters"), e);
@@ -3157,7 +3156,7 @@ public class Request implements HttpServletRequest {
             success = true;
         } finally {
             if (!success) {
-                parameters.setParseFailedReason(FailReason.UNKNOWN);
+             // TODO parameters.setParseFailedReason(FailReason.UNKNOWN);
             }
         }
 
@@ -3347,6 +3346,7 @@ public class Request implements HttpServletRequest {
                 // NO-OP
             }
         });
+        /*
         specialAttributes.put(Globals.PARAMETER_PARSE_FAILED_ATTR, new SpecialAttributeAdapter() {
             @Override
             public Object get(Request request, String name) {
@@ -3361,6 +3361,8 @@ public class Request implements HttpServletRequest {
                 // NO-OP
             }
         });
+        */
+        /*
         specialAttributes.put(Globals.PARAMETER_PARSE_FAILED_REASON_ATTR, new SpecialAttributeAdapter() {
             @Override
             public Object get(Request request, String name) {
@@ -3372,6 +3374,7 @@ public class Request implements HttpServletRequest {
                 // NO-OP
             }
         });
+        */
         specialAttributes.put(Globals.SENDFILE_SUPPORTED_ATTR, new SpecialAttributeAdapter() {
             @Override
             public Object get(Request request, String name) {
diff --git a/java/org/apache/catalina/core/StandardHostValve.java b/java/org/apache/catalina/core/StandardHostValve.java
index 9814a3cc28..98b8e92a2d 100644
--- a/java/org/apache/catalina/core/StandardHostValve.java
+++ b/java/org/apache/catalina/core/StandardHostValve.java
@@ -37,6 +37,7 @@ import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
 import org.apache.tomcat.util.ExceptionUtils;
 import org.apache.tomcat.util.descriptor.web.ErrorPage;
+import org.apache.tomcat.util.http.InvalidParameterException;
 import org.apache.tomcat.util.res.StringManager;
 
 /**
@@ -294,7 +295,7 @@ final class StandardHostValve extends ValveBase {
                     }
                 }
             }
-        } else {
+        } else if (!(throwable instanceof InvalidParameterException)){
             // A custom error-page has not been defined for the exception
             // that was thrown during request processing. Check if an
             // error-page for error code 500 was specified and if so,
diff --git a/java/org/apache/catalina/core/StandardWrapperValve.java b/java/org/apache/catalina/core/StandardWrapperValve.java
index 5426004930..4fd4279125 100644
--- a/java/org/apache/catalina/core/StandardWrapperValve.java
+++ b/java/org/apache/catalina/core/StandardWrapperValve.java
@@ -38,6 +38,7 @@ import org.apache.catalina.valves.ValveBase;
 import org.apache.coyote.CloseNowException;
 import org.apache.tomcat.util.ExceptionUtils;
 import org.apache.tomcat.util.buf.MessageBytes;
+import org.apache.tomcat.util.http.InvalidParameterException;
 import org.apache.tomcat.util.log.SystemLogHandler;
 import org.apache.tomcat.util.res.StringManager;
 
@@ -168,6 +169,8 @@ final class StandardWrapperValve extends ValveBase {
                 }
 
             }
+        } catch (InvalidParameterException e) {
+            exception(request, response, e, HttpServletResponse.SC_BAD_REQUEST);
         } catch (ClientAbortException | CloseNowException e) {
             if (container.getLogger().isDebugEnabled()) {
                 container.getLogger().debug(
@@ -271,8 +274,21 @@ final class StandardWrapperValve extends ValveBase {
      * @param exception The exception that occurred (which possibly wraps a root cause exception
      */
     private void exception(Request request, Response response, Throwable exception) {
+        exception(request, response, exception, HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+    }
+
+    /**
+     * Handle the specified ServletException encountered while processing the specified Request to produce the specified
+     * Response. Any exceptions that occur during generation of the exception report are logged and swallowed.
+     *
+     * @param request   The request being processed
+     * @param response  The response being generated
+     * @param exception The exception that occurred (which possibly wraps a root cause exception
+     * @param status    The HTTP status code to use for the response
+     */
+    private void exception(Request request, Response response, Throwable exception, int status) {
         request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, exception);
-        response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR);
+        response.setStatus(status);
         response.setError();
     }
 
diff --git a/java/org/apache/tomcat/util/http/InvalidParameterException.java b/java/org/apache/tomcat/util/http/InvalidParameterException.java
new file mode 100644
index 0000000000..95c5806be6
--- /dev/null
+++ b/java/org/apache/tomcat/util/http/InvalidParameterException.java
@@ -0,0 +1,56 @@
+/*
+ *  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.http;
+
+/**
+ * Extend {@link IllegalStateException} to identify the cause as an invalid parameter.
+ */
+public class InvalidParameterException extends IllegalStateException {
+
+    private static final long serialVersionUID = 1L;
+
+
+    public InvalidParameterException() {
+        super();
+    }
+
+
+    public InvalidParameterException(String message, Throwable cause) {
+        super(message, cause);
+    }
+
+
+    public InvalidParameterException(String s) {
+        super(s);
+    }
+
+
+    public InvalidParameterException(Throwable cause) {
+        super(cause);
+    }
+
+    @Override
+    public synchronized Throwable fillInStackTrace() {
+        /*
+         * This exception is triggered by user input and therefore, since generating stack traces is relatively
+         * expensive, stack traces have been disabled for this class. There should be enough information in the
+         * exception message to identify the problematic parameter. If not, it is very likely that fixing the message is
+         * a better fix than enabling stack traces.
+         */
+        return this;
+    }
+}
diff --git a/java/org/apache/tomcat/util/http/LocalStrings.properties b/java/org/apache/tomcat/util/http/LocalStrings.properties
index 43307a8893..368adf136a 100644
--- a/java/org/apache/tomcat/util/http/LocalStrings.properties
+++ b/java/org/apache/tomcat/util/http/LocalStrings.properties
@@ -24,17 +24,16 @@ headers.maxCountFail=More than the maximum allowed number of headers, [{0}], wer
 
 parameters.bytes=Start processing with input [{0}]
 parameters.copyFail=Failed to create copy of original parameter values for debug logging purposes
-parameters.decodeFail.debug=Character decoding failed. Parameter [{0}] with value [{1}] has been ignored.
-parameters.decodeFail.info=Character decoding failed. Parameter [{0}] with value [{1}] has been ignored. Note that the name and value quoted here may be corrupted due to the failed decoding. Use debug level logging to see the original, non-corrupted values.
+parameters.corrupted=Note that the name and/or value quoted here may be corrupted due to the failed decoding. Use debug level logging to see the original, non-corrupted values.
+parameters.decodeFail=Character decoding failed for parameter [{0}] with value [{1}].
 parameters.emptyChunk=Empty parameter chunk ignored
 parameters.fallToDebug=\n\
 \ Note: further occurrences of Parameter errors will be logged at DEBUG level.
 parameters.invalidChunk=Invalid chunk starting at byte [{0}] and ending at byte [{1}] with a value of [{2}] ignored
 parameters.maxCountFail=More than the maximum number of request parameters (GET plus POST) for a single request ([{0}]) were detected. Any parameters beyond this limit have been ignored. To change this limit, set the maxParameterCount attribute on the Connector.
-parameters.maxCountFail.fallToDebug=\n\
-\ Note: further occurrences of this error will be logged at DEBUG level.
 parameters.multipleDecodingFail=Character decoding failed. A total of [{0}] failures were detected but only the first was logged. Enable debug level logging for this logger to log all failures.
 parameters.noequal=Parameter starting at position [{0}] and ending at position [{1}] with a value of [{2}] was not followed by an ''='' character
+parameters.urlDecodeFail=URL (%nn) decoding failed for parameter [{0}] with value [{1}].
 
 rfc6265CookieProcessor.invalidAttributeName=An invalid attribute name [{0}] was specified for this cookie
 rfc6265CookieProcessor.invalidAttributeValue=An invalid attribute value [{1}] was specified for this cookie attribute [{0}]
diff --git a/java/org/apache/tomcat/util/http/ParameterErrorHandling.java b/java/org/apache/tomcat/util/http/ParameterErrorHandling.java
new file mode 100644
index 0000000000..3d139cfe6b
--- /dev/null
+++ b/java/org/apache/tomcat/util/http/ParameterErrorHandling.java
@@ -0,0 +1,100 @@
+/*
+ * 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.http;
+
+import java.nio.charset.CodingErrorAction;
+
+public class ParameterErrorHandling {
+
+    private boolean skipEmptyParameter = false;
+    private boolean skipInvalidParameter = false;
+    private boolean skipUrlDecodingError = false;
+    private CodingErrorAction malformedInputAction = CodingErrorAction.REPORT;
+    private CodingErrorAction unmappableCharacterAction = CodingErrorAction.REPORT;
+    private boolean skipDecodingError = false;
+    private boolean skipMaxParameterCountError = false;
+
+
+    public boolean getSkipEmptyParameter() {
+        return skipEmptyParameter;
+    }
+
+
+    public void setSkipEmptyParameter(boolean skipEmptyParameter) {
+        this.skipEmptyParameter = skipEmptyParameter;
+    }
+
+
+    public boolean getSkipInvalidParameter() {
+        return skipInvalidParameter;
+    }
+
+
+    public void setSkipInvalidParameter(boolean skipInvalidParameter) {
+        this.skipInvalidParameter = skipInvalidParameter;
+    }
+
+
+    public boolean getSkipUrlDecodingError() {
+        return skipUrlDecodingError;
+    }
+
+
+    public void setIgnoreUrlDecodingError(boolean skipUrlDecodingError) {
+        this.skipUrlDecodingError = skipUrlDecodingError;
+    }
+
+
+    public CodingErrorAction malformedInputAction() {
+        return malformedInputAction;
+    }
+
+
+    public void onMalformedInput(CodingErrorAction action) {
+        this.malformedInputAction = action;
+    }
+
+
+    public CodingErrorAction unmappableCharacterAction() {
+        return unmappableCharacterAction;
+    }
+
+
+    public void onUnmappableCharacter(CodingErrorAction action) {
+        this.unmappableCharacterAction = action;
+    }
+
+
+    public boolean getSkipDecodingError() {
+        return skipDecodingError;
+    }
+
+
+    public void setSkipDecodingError(boolean skipDecodingError) {
+        this.skipDecodingError = skipDecodingError;
+    }
+
+
+    public boolean getSkipMaxParameterCountError() {
+        return skipMaxParameterCountError;
+    }
+
+
+    public void setSkipMaxParameterCountError(boolean skipMaxParameterCountError) {
+        this.skipMaxParameterCountError = skipMaxParameterCountError;
+    }
+}
diff --git a/java/org/apache/tomcat/util/http/Parameters.java b/java/org/apache/tomcat/util/http/Parameters.java
index e56793b3de..bb12f771af 100644
--- a/java/org/apache/tomcat/util/http/Parameters.java
+++ b/java/org/apache/tomcat/util/http/Parameters.java
@@ -17,6 +17,7 @@
 package org.apache.tomcat.util.http;
 
 import java.io.IOException;
+import java.nio.charset.CharacterCodingException;
 import java.nio.charset.Charset;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
@@ -24,6 +25,7 @@ import java.util.Collections;
 import java.util.Enumeration;
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.function.BooleanSupplier;
 
 import org.apache.juli.logging.Log;
 import org.apache.juli.logging.LogFactory;
@@ -38,7 +40,7 @@ public final class Parameters {
 
     private static final Log log = LogFactory.getLog(Parameters.class);
 
-    private static final UserDataHelper userDataLog = new UserDataHelper(log);
+    private static final UserDataHelper paramParsingLog = new UserDataHelper(log);
 
     private static final UserDataHelper maxParamCountLog = new UserDataHelper(log);
 
@@ -58,11 +60,8 @@ public final class Parameters {
     private int limit = -1;
     private int parameterCount = 0;
 
-    /**
-     * Set to the reason for the failure (the first failure if there is more than one) if there were failures during
-     * parameter parsing.
-     */
-    private FailReason parseFailedReason = null;
+    private final ParameterErrorHandling errorHandler = new ParameterErrorHandling();
+    private int parseFailureCount = 0;
 
     public Parameters() {
         // NO-OP
@@ -102,23 +101,6 @@ public final class Parameters {
     }
 
 
-    public boolean isParseFailed() {
-        return parseFailedReason != null;
-    }
-
-
-    public FailReason getParseFailedReason() {
-        return parseFailedReason;
-    }
-
-
-    public void setParseFailedReason(FailReason failReason) {
-        if (this.parseFailedReason == null) {
-            this.parseFailedReason = failReason;
-        }
-    }
-
-
     public int size() {
         return parameterCount;
     }
@@ -130,7 +112,7 @@ public final class Parameters {
         didQueryParameters = false;
         charset = DEFAULT_BODY_CHARSET;
         decodedQuery.recycle();
-        parseFailedReason = null;
+        parseFailureCount = 0;
     }
 
 
@@ -196,7 +178,6 @@ public final class Parameters {
 
 
     public void addParameter(String key, String value) throws IllegalStateException {
-
         if (key == null) {
             return;
         }
@@ -204,18 +185,20 @@ public final class Parameters {
         if (limit > -1 && parameterCount >= limit) {
             // Processing this parameter will push us over the limit. ISE is
             // what Request.parseParts() uses for requests that are too big
-            setParseFailedReason(FailReason.TOO_MANY_PARAMETERS);
-            throw new IllegalStateException(sm.getString("parameters.maxCountFail", Integer.valueOf(limit)));
+            String msg = sm.getString("parameters.maxCountFail", Integer.valueOf(limit));
+            handleParameterProcessingError(msg, maxParamCountLog, () -> errorHandler.getSkipMaxParameterCountError(),
+                    null);
         }
         parameterCount++;
-
         paramHashValues.computeIfAbsent(key, k -> new ArrayList<>(1)).add(value);
     }
 
+
     public void setURLDecoder(UDecoder u) {
         urlDec = u;
     }
 
+
     // -------------------- Parameter parsing --------------------
     // we are called from a single thread - we can do it the hard way
     // if needed
@@ -237,8 +220,6 @@ public final class Parameters {
             log.debug(sm.getString("parameters.bytes", new String(bytes, start, len, DEFAULT_BODY_CHARSET)));
         }
 
-        int decodeFailCount = 0;
-
         int pos = start;
         int end = start + len;
 
@@ -309,37 +290,21 @@ public final class Parameters {
             if (nameEnd <= nameStart) {
                 if (valueStart == -1) {
                     // &&
-                    if (log.isDebugEnabled()) {
-                        log.debug(sm.getString("parameters.emptyChunk"));
-                    }
-                    // Do not flag as error
-                    continue;
+                    String msg = sm.getString("parameters.emptyChunk");
+                    handleParameterProcessingError(msg, paramParsingLog, () -> errorHandler.getSkipEmptyParameter(),
+                            null);
                 }
                 // &=foo&
-                UserDataHelper.Mode logMode = userDataLog.getNextMode();
-                if (logMode != null) {
-                    String extract;
-                    if (valueEnd > nameStart) {
-                        extract = new String(bytes, nameStart, valueEnd - nameStart, DEFAULT_BODY_CHARSET);
-                    } else {
-                        extract = "";
-                    }
-                    String message = sm.getString("parameters.invalidChunk", Integer.valueOf(nameStart),
-                            Integer.valueOf(valueEnd), extract);
-                    switch (logMode) {
-                        case INFO_THEN_DEBUG:
-                            message += sm.getString("parameters.fallToDebug");
-                            //$FALL-THROUGH$
-                        case INFO:
-                            log.info(message);
-                            break;
-                        case DEBUG:
-                            log.debug(message);
-                    }
+                String extract;
+                if (valueEnd > nameStart) {
+                    extract = new String(bytes, nameStart, valueEnd - nameStart, DEFAULT_BODY_CHARSET);
+                } else {
+                    extract = "";
                 }
-                setParseFailedReason(FailReason.NO_NAME);
-                continue;
-                // invalid chunk - it's better to ignore
+                String msg = sm.getString("parameters.invalidChunk", Integer.valueOf(nameStart),
+                        Integer.valueOf(valueEnd), extract);
+                handleParameterProcessingError(msg, paramParsingLog, () -> errorHandler.getSkipInvalidParameter(),
+                        null);
             }
 
             tmpName.setBytes(bytes, nameStart, nameEnd - nameStart);
@@ -366,88 +331,73 @@ public final class Parameters {
                 }
             }
 
-            try {
-                String name;
-                String value;
+            String name = null;
+            String value = null;
 
+            try {
                 if (decodeName) {
-                    urlDecode(tmpName);
+                    try {
+                        urlDecode(tmpName);
+                    } catch (IOException e) {
+                        // Invalid %nn sequence
+                        String msg = getParameterMessage("parameters.urlDecodeFail");
+                        handleParameterProcessingError(msg, paramParsingLog,
+                                () -> errorHandler.getSkipUrlDecodingError(), null);
+                    }
                 }
+
                 tmpName.setCharset(charset);
-                name = tmpName.toString();
+                try {
+                    name = tmpName.toString(errorHandler.malformedInputAction(),
+                            errorHandler.unmappableCharacterAction());
+                } catch (CharacterCodingException e) {
+                    // Invalid byte sequence for character set
+                    String msg = getParameterMessage("parameters.decodeFail");
+                    handleParameterProcessingError(msg, paramParsingLog, () -> errorHandler.getSkipDecodingError(),
+                            null);
+                }
 
                 if (valueStart >= 0) {
                     if (decodeValue) {
-                        urlDecode(tmpValue);
+                        try {
+                            urlDecode(tmpValue);
+                        } catch (IOException e) {
+                            // Invalid %nn sequence
+                            String msg = getParameterMessage("parameters.urlDecodeFail");
+                            handleParameterProcessingError(msg, paramParsingLog,
+                                    () -> errorHandler.getSkipUrlDecodingError(), null);
+                        }
                     }
                     tmpValue.setCharset(charset);
-                    value = tmpValue.toString();
+                    try {
+                        value = tmpValue.toString(errorHandler.malformedInputAction(),
+                                errorHandler.unmappableCharacterAction());
+                    } catch (CharacterCodingException e) {
+                        // Invalid byte sequence for character set
+                        String msg = getParameterMessage("parameters.decodeFail");
+                        handleParameterProcessingError(msg, paramParsingLog, () -> errorHandler.getSkipDecodingError(),
+                                null);
+                    }
                 } else {
                     value = "";
                 }
 
-                try {
-                    addParameter(name, value);
-                } catch (IllegalStateException ise) {
-                    // Hitting limit stops processing further params but does
-                    // not cause request to fail.
-                    UserDataHelper.Mode logMode = maxParamCountLog.getNextMode();
-                    if (logMode != null) {
-                        String message = ise.getMessage();
-                        switch (logMode) {
-                            case INFO_THEN_DEBUG:
-                                message += sm.getString("parameters.maxCountFail.fallToDebug");
-                                //$FALL-THROUGH$
-                            case INFO:
-                                log.info(message);
-                                break;
-                            case DEBUG:
-                                log.debug(message);
-                        }
-                    }
-                    break;
-                }
-            } catch (IOException e) {
-                setParseFailedReason(FailReason.URL_DECODING);
-                decodeFailCount++;
-                if (decodeFailCount == 1 || log.isDebugEnabled()) {
-                    if (log.isDebugEnabled()) {
-                        log.debug(
-                                sm.getString("parameters.decodeFail.debug", origName.toString(), origValue.toString()),
-                                e);
-                    } else if (log.isInfoEnabled()) {
-                        UserDataHelper.Mode logMode = userDataLog.getNextMode();
-                        if (logMode != null) {
-                            String message =
-                                    sm.getString("parameters.decodeFail.info", tmpName.toString(), tmpValue.toString());
-                            switch (logMode) {
-                                case INFO_THEN_DEBUG:
-                                    message += sm.getString("parameters.fallToDebug");
-                                    //$FALL-THROUGH$
-                                case INFO:
-                                    log.info(message);
-                                    break;
-                                case DEBUG:
-                                    log.debug(message);
-                            }
-                        }
-                    }
+                addParameter(name, value);
+            } finally {
+                tmpName.recycle();
+                tmpValue.recycle();
+                // Only recycle copies if we used them
+                if (log.isDebugEnabled()) {
+                    origName.recycle();
+                    origValue.recycle();
                 }
             }
-
-            tmpName.recycle();
-            tmpValue.recycle();
-            // Only recycle copies if we used them
-            if (log.isDebugEnabled()) {
-                origName.recycle();
-                origValue.recycle();
-            }
         }
 
-        if (decodeFailCount > 1 && !log.isDebugEnabled()) {
-            UserDataHelper.Mode logMode = userDataLog.getNextMode();
+        if (parseFailureCount > 1 && !log.isDebugEnabled()) {
+            UserDataHelper.Mode logMode = paramParsingLog.getNextMode();
             if (logMode != null) {
-                String message = sm.getString("parameters.multipleDecodingFail", Integer.valueOf(decodeFailCount));
+                String message = sm.getString("parameters.multipleDecodingFail", Integer.valueOf(parseFailureCount));
                 switch (logMode) {
                     case INFO_THEN_DEBUG:
                         message += sm.getString("parameters.fallToDebug");
@@ -456,12 +406,57 @@ public final class Parameters {
                         log.info(message);
                         break;
                     case DEBUG:
-                        log.debug(message);
+                        // NO-OP: If debug is enabled all failures will have been logged.
                 }
             }
         }
     }
 
+
+    private String getParameterMessage(String messageKey) {
+        // Note: The conversions here won't fail because toString() always uses CodingErrorAction.REPLACE
+        if (log.isDebugEnabled()) {
+            return sm.getString(messageKey, origName.toString(), origValue.toString());
+        } else {
+            return sm.getString(messageKey, tmpName.toString(), tmpValue.toString()) + " " +
+                    sm.getString("parameters.corrupted");
+        }
+    }
+
+
+    private void handleParameterProcessingError(String message, UserDataHelper userDataHelper,
+            BooleanSupplier skipError, Throwable cause) {
+        parseFailureCount++;
+        if (log.isDebugEnabled()) {
+            log.debug(message);
+        } else {
+            if (parseFailureCount == 1) {
+                UserDataHelper.Mode logMode = userDataHelper.getNextMode();
+                if (logMode != null) {
+                    switch (logMode) {
+                        case INFO_THEN_DEBUG:
+                            log.info(message + sm.getString("parameters.fallToDebug"));
+                            break;
+                        case INFO:
+                            log.info(message);
+                            break;
+                        case DEBUG:
+                            // NO-OP: If debug is enabled the message will be logged above
+                    }
+                }
+            }
+        }
+        if (skipError.getAsBoolean()) {
+            return;
+        }
+        if (cause == null) {
+            throw new InvalidParameterException(message);
+        } else {
+            throw new InvalidParameterException(message, cause);
+        }
+    }
+
+
     private void urlDecode(ByteChunk bc) throws IOException {
         if (urlDec == null) {
             urlDec = new UDecoder();


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