diff --git a/java/org/apache/catalina/connector/Request.java b/java/org/apache/catalina/connector/Request.java index 9a6de071d564..869cae087d67 100644 --- a/java/org/apache/catalina/connector/Request.java +++ b/java/org/apache/catalina/connector/Request.java @@ -103,8 +103,8 @@ 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.InvalidParameterException; 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; @@ -303,6 +303,12 @@ public org.apache.coyote.Request getCoyoteRequest() { protected ParameterMap parameterMap = new ParameterMap<>(); + /** + * The exception thrown, if any when parsing the parameters including parts. + */ + protected IllegalStateException parametersParseException = null; + + /** * The parts, if any, uploaded with this request. */ @@ -445,6 +451,7 @@ public void recycle() { } parts = null; } + parametersParseException = null; partsParseException = null; locales.clear(); localesParsed = false; @@ -1061,30 +1068,13 @@ public Enumeration getLocales() { } - /** - * @return the value of the specified request parameter, if any; otherwise, return null. If there is - * more than one value defined, return only the first one. - * - * @param name Name of the desired request parameter - */ @Override public String getParameter(String name) { - - if (!parametersParsed) { - parseParameters(); - } - + parseParameters(); return coyoteRequest.getParameters().getParameter(name); - } - /** - * Returns a Map of the parameters of this request. Request parameters are extra information sent with - * the request. For HTTP servlets, parameters are contained in the query string or posted form data. - * - * @return A Map containing parameter names as keys and parameter values as map values. - */ @Override public Map getParameterMap() { @@ -1102,39 +1092,20 @@ public Map getParameterMap() { parameterMap.setLocked(true); return parameterMap; - } - /** - * @return the names of all defined request parameters for this request. - */ @Override public Enumeration getParameterNames() { - - if (!parametersParsed) { - parseParameters(); - } - + parseParameters(); return coyoteRequest.getParameters().getParameterNames(); - } - /** - * @return the defined values for the specified request parameter, if any; otherwise, return null. - * - * @param name Name of the desired request parameter - */ @Override public String[] getParameterValues(String name) { - - if (!parametersParsed) { - parseParameters(); - } - + parseParameters(); return coyoteRequest.getParameters().getParameterValues(name); - } @@ -2635,6 +2606,7 @@ public void logout() throws ServletException { getContext().getAuthenticator().logout(this); } + @Override public Collection getParts() throws IOException, IllegalStateException, ServletException { @@ -2653,6 +2625,7 @@ public Collection getParts() throws IOException, IllegalStateException, Se return parts; } + private void parseParts() { // Return immediately if the parts have already been parsed @@ -2677,119 +2650,103 @@ private void parseParts() { Parameters parameters = coyoteRequest.getParameters(); parameters.setLimit(maxParameterCount); - boolean success = false; - try { - File location; - String locationStr = mce.getLocation(); - if (locationStr == null || locationStr.length() == 0) { - location = ((File) context.getServletContext().getAttribute(ServletContext.TEMPDIR)); - } else { - // If relative, it is relative to TEMPDIR - location = new File(locationStr); - if (!location.isAbsolute()) { - location = new File((File) context.getServletContext().getAttribute(ServletContext.TEMPDIR), - locationStr).getAbsoluteFile(); - } - } - - if (!location.exists() && context.getCreateUploadTargets()) { - log.warn(sm.getString("coyoteRequest.uploadCreate", location.getAbsolutePath(), - getMappingData().wrapper.getName())); - if (!location.mkdirs()) { - log.warn(sm.getString("coyoteRequest.uploadCreateFail", location.getAbsolutePath())); - } + File location; + String locationStr = mce.getLocation(); + if (locationStr == null || locationStr.length() == 0) { + location = ((File) context.getServletContext().getAttribute(ServletContext.TEMPDIR)); + } else { + // If relative, it is relative to TEMPDIR + location = new File(locationStr); + if (!location.isAbsolute()) { + location = + new File((File) context.getServletContext().getAttribute(ServletContext.TEMPDIR), locationStr) + .getAbsoluteFile(); } + } - if (!location.isDirectory()) { - parameters.setParseFailedReason(FailReason.MULTIPART_CONFIG_INVALID); - partsParseException = new IOException(sm.getString("coyoteRequest.uploadLocationInvalid", location)); - return; + if (!location.exists() && context.getCreateUploadTargets()) { + log.warn(sm.getString("coyoteRequest.uploadCreate", location.getAbsolutePath(), + getMappingData().wrapper.getName())); + if (!location.mkdirs()) { + log.warn(sm.getString("coyoteRequest.uploadCreateFail", location.getAbsolutePath())); } + } + if (!location.isDirectory()) { + partsParseException = new IOException(sm.getString("coyoteRequest.uploadLocationInvalid", location)); + return; + } - // Create a new file upload handler - DiskFileItemFactory factory = new DiskFileItemFactory(); - try { - factory.setRepository(location.getCanonicalFile()); - } catch (IOException ioe) { - parameters.setParseFailedReason(FailReason.IO_ERROR); - partsParseException = ioe; - return; - } - factory.setSizeThreshold(mce.getFileSizeThreshold()); + // Create a new file upload handler + DiskFileItemFactory factory = new DiskFileItemFactory(); + try { + factory.setRepository(location.getCanonicalFile()); + } catch (IOException ioe) { + partsParseException = ioe; + return; + } + factory.setSizeThreshold(mce.getFileSizeThreshold()); - FileUpload upload = new FileUpload(); - upload.setFileItemFactory(factory); - upload.setFileSizeMax(mce.getMaxFileSize()); - upload.setSizeMax(mce.getMaxRequestSize()); - if (maxParameterCount > -1) { - // There is a limit. The limit for parts needs to be reduced by - // the number of parameters we have already parsed. - // Must be under the limit else parsing parameters would have - // triggered an exception. - upload.setFileCountMax(maxParameterCount - parameters.size()); - } + FileUpload upload = new FileUpload(); + upload.setFileItemFactory(factory); + upload.setFileSizeMax(mce.getMaxFileSize()); + upload.setSizeMax(mce.getMaxRequestSize()); + if (maxParameterCount > -1) { + // There is a limit. The limit for parts needs to be reduced by + // the number of parameters we have already parsed. + // Must be under the limit else parsing parameters would have + // triggered an exception. + upload.setFileCountMax(maxParameterCount - parameters.size()); + } - parts = new ArrayList<>(); - try { - List items = upload.parseRequest(new ServletRequestContext(this)); - int maxPostSize = getConnector().getMaxPostSize(); - int postSize = 0; - Charset charset = getCharset(); - for (FileItem item : items) { - ApplicationPart part = new ApplicationPart(item, location); - parts.add(part); - if (part.getSubmittedFileName() == null) { - String name = part.getName(); - if (maxPostSize >= 0) { - // Have to calculate equivalent size. Not completely - // accurate but close enough. - postSize += name.getBytes(charset).length; - // Equals sign - postSize++; - // Value length - postSize += part.getSize(); - // Value separator - postSize++; - if (postSize > maxPostSize) { - parameters.setParseFailedReason(FailReason.POST_TOO_LARGE); - throw new IllegalStateException(sm.getString("coyoteRequest.maxPostSizeExceeded")); - } - } - String value = null; - try { - value = part.getString(charset.name()); - } catch (UnsupportedEncodingException uee) { - // Not possible + parts = new ArrayList<>(); + try { + List items = upload.parseRequest(new ServletRequestContext(this)); + int maxPostSize = getConnector().getMaxPostSize(); + int postSize = 0; + Charset charset = getCharset(); + for (FileItem item : items) { + ApplicationPart part = new ApplicationPart(item, location); + parts.add(part); + if (part.getSubmittedFileName() == null) { + String name = part.getName(); + if (maxPostSize >= 0) { + // Have to calculate equivalent size. Not completely + // accurate but close enough. + postSize += name.getBytes(charset).length; + // Equals sign + postSize++; + // Value length + postSize += part.getSize(); + // Value separator + postSize++; + if (postSize > maxPostSize) { + throw new IllegalStateException(sm.getString("coyoteRequest.maxPostSizeExceeded")); } - parameters.addParameter(name, value); } + String value = null; + try { + value = part.getString(charset.name()); + } catch (UnsupportedEncodingException uee) { + // Not possible + } + parameters.addParameter(name, value); } - - success = true; - } catch (InvalidContentTypeException e) { - parameters.setParseFailedReason(FailReason.INVALID_CONTENT_TYPE); - partsParseException = new ServletException(e); - } catch (SizeException e) { - parameters.setParseFailedReason(FailReason.POST_TOO_LARGE); - checkSwallowInput(); - partsParseException = new IllegalStateException(e); - } catch (IOException e) { - parameters.setParseFailedReason(FailReason.IO_ERROR); - partsParseException = e; - } catch (IllegalStateException e) { - // addParameters() will set parseFailedReason - checkSwallowInput(); - partsParseException = e; - } - } finally { - // This might look odd but is correct. setParseFailedReason() only - // sets the failure reason if none is currently set. This code could - // 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); } + } catch (InvalidContentTypeException e) { + partsParseException = new ServletException(e); + return; + } catch (SizeException e) { + checkSwallowInput(); + partsParseException = new InvalidParameterException(e, HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE); + return; + } catch (IOException e) { + partsParseException = e; + return; + } catch (IllegalStateException e) { + checkSwallowInput(); + partsParseException = e; + return; } } @@ -3012,131 +2969,142 @@ protected void convertCookies() { * Parse request parameters. */ protected void parseParameters() { + doParseParameters(); + if (parametersParseException != null) { + throw parametersParseException; + } + } + + + protected void doParseParameters() { + if (parametersParsed) { + return; + } parametersParsed = true; Parameters parameters = coyoteRequest.getParameters(); - boolean success = false; - try { - // Set this every time in case limit has been changed via JMX - int maxParameterCount = getConnector().getMaxParameterCount(); - if (parts != null && maxParameterCount > 0) { - maxParameterCount -= parts.size(); - } - parameters.setLimit(maxParameterCount); - // getCharacterEncoding() may have been overridden to search for - // hidden form field containing request encoding - Charset charset = getCharset(); + // Set this every time in case limit has been changed via JMX + int maxParameterCount = getConnector().getMaxParameterCount(); + if (parts != null && maxParameterCount > 0) { + maxParameterCount -= parts.size(); + } + parameters.setLimit(maxParameterCount); - boolean useBodyEncodingForURI = connector.getUseBodyEncodingForURI(); - parameters.setCharset(charset); - if (useBodyEncodingForURI) { - parameters.setQueryStringCharset(charset); - } - // Note: If !useBodyEncodingForURI, the query string encoding is - // that set towards the start of CoyoteAdapter.service() + // getCharacterEncoding() may have been overridden to search for + // hidden form field containing request encoding + Charset charset = getCharset(); - parameters.handleQueryParameters(); + boolean useBodyEncodingForURI = connector.getUseBodyEncodingForURI(); + parameters.setCharset(charset); + if (useBodyEncodingForURI) { + parameters.setQueryStringCharset(charset); + } + // Note: If !useBodyEncodingForURI, the query string encoding is + // that set towards the start of CoyoteAdapter.service() - if (usingInputStream || usingReader) { - success = true; - return; - } + parameters.handleQueryParameters(); - String contentType = getContentType(); - if (contentType == null) { - contentType = ""; - } - int semicolon = contentType.indexOf(';'); - if (semicolon >= 0) { - contentType = contentType.substring(0, semicolon).trim(); - } else { - contentType = contentType.trim(); - } + if (usingInputStream || usingReader) { + return; + } - if ("multipart/form-data".equals(contentType)) { - parseParts(); - success = true; - return; - } + String contentType = getContentType(); + if (contentType == null) { + contentType = ""; + } + int semicolon = contentType.indexOf(';'); + if (semicolon >= 0) { + contentType = contentType.substring(0, semicolon).trim(); + } else { + contentType = contentType.trim(); + } - if (!getConnector().isParseBodyMethod(getMethod())) { - success = true; - return; + if ("multipart/form-data".equals(contentType)) { + parseParts(); + if (partsParseException instanceof IllegalStateException) { + parametersParseException = (IllegalStateException) partsParseException; + } else if (partsParseException != null) { + parametersParseException = new InvalidParameterException(partsParseException); } + return; + } - if (!("application/x-www-form-urlencoded".equals(contentType))) { - success = true; - return; - } + if (!getConnector().isParseBodyMethod(getMethod())) { + return; + } - int len = getContentLength(); + if (!("application/x-www-form-urlencoded".equals(contentType))) { + return; + } - if (len > 0) { - int maxPostSize = connector.getMaxPostSize(); - if ((maxPostSize >= 0) && (len > maxPostSize)) { - Context context = getContext(); - if (context != null && context.getLogger().isDebugEnabled()) { - context.getLogger().debug(sm.getString("coyoteRequest.postTooLarge")); - } - checkSwallowInput(); - parameters.setParseFailedReason(FailReason.POST_TOO_LARGE); - return; + int len = getContentLength(); + + if (len > 0) { + int maxPostSize = connector.getMaxPostSize(); + if ((maxPostSize >= 0) && (len > maxPostSize)) { + String message = sm.getString("coyoteRequest.postTooLarge"); + Context context = getContext(); + if (context != null && context.getLogger().isDebugEnabled()) { + context.getLogger().debug(message); } - byte[] formData = null; - if (len < CACHED_POST_LEN) { - if (postData == null) { - postData = new byte[CACHED_POST_LEN]; - } - formData = postData; - } else { - formData = new byte[len]; + checkSwallowInput(); + parametersParseException = + new InvalidParameterException(message, HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE); + return; + } + byte[] formData = null; + if (len < CACHED_POST_LEN) { + if (postData == null) { + postData = new byte[CACHED_POST_LEN]; } - try { - readPostBodyFully(formData, len); - } catch (IOException e) { - // Client disconnect - Context context = getContext(); - if (context != null && context.getLogger().isDebugEnabled()) { - context.getLogger().debug(sm.getString("coyoteRequest.parseParameters"), e); - } - parameters.setParseFailedReason(FailReason.CLIENT_DISCONNECT); - return; + formData = postData; + } else { + formData = new byte[len]; + } + try { + readPostBodyFully(formData, len); + } catch (IOException e) { + // Client disconnect + Context context = getContext(); + if (context != null && context.getLogger().isDebugEnabled()) { + context.getLogger().debug(sm.getString("coyoteRequest.parseParameters"), e); } - parameters.processParameters(formData, 0, len); - } else if ("chunked".equalsIgnoreCase(coyoteRequest.getHeader("transfer-encoding"))) { - byte[] formData = null; - try { - formData = readChunkedPostBody(); - } catch (IllegalStateException ise) { - // chunkedPostTooLarge error - parameters.setParseFailedReason(FailReason.POST_TOO_LARGE); - Context context = getContext(); - if (context != null && context.getLogger().isDebugEnabled()) { - context.getLogger().debug(sm.getString("coyoteRequest.parseParameters"), ise); - } - return; - } catch (IOException e) { - // Client disconnect - parameters.setParseFailedReason(FailReason.CLIENT_DISCONNECT); - Context context = getContext(); - if (context != null && context.getLogger().isDebugEnabled()) { - context.getLogger().debug(sm.getString("coyoteRequest.parseParameters"), e); - } - return; + response.getCoyoteResponse().action(ActionCode.CLOSE_NOW, null); + if (e instanceof ClientAbortException) { + parametersParseException = new InvalidParameterException(e); + } else { + parametersParseException = new InvalidParameterException(new ClientAbortException(e)); + } + return; + } + parameters.processParameters(formData, 0, len); + } else if ("chunked".equalsIgnoreCase(coyoteRequest.getHeader("transfer-encoding"))) { + byte[] formData = null; + try { + formData = readChunkedPostBody(); + } catch (IllegalStateException ise) { + parametersParseException = ise; + return; + } catch (IOException e) { + // Client disconnect + Context context = getContext(); + if (context != null && context.getLogger().isDebugEnabled()) { + context.getLogger().debug(sm.getString("coyoteRequest.parseParameters"), e); } - if (formData != null) { - parameters.processParameters(formData, 0, formData.length); + response.getCoyoteResponse().action(ActionCode.CLOSE_NOW, null); + if (e instanceof ClientAbortException) { + parametersParseException = new InvalidParameterException(e); + } else { + parametersParseException = new InvalidParameterException(new ClientAbortException(e)); } + return; } - success = true; - } finally { - if (!success) { - parameters.setParseFailedReason(FailReason.UNKNOWN); + if (formData != null) { + parameters.processParameters(formData, 0, formData.length); } } - } @@ -3178,7 +3146,8 @@ protected byte[] readChunkedPostBody() throws IOException { if (connector.getMaxPostSize() >= 0 && (body.getLength() + len) > connector.getMaxPostSize()) { // Too much data checkSwallowInput(); - throw new IllegalStateException(sm.getString("coyoteRequest.chunkedPostTooLarge")); + throw new InvalidParameterException(sm.getString("coyoteRequest.chunkedPostTooLarge"), + HttpServletResponse.SC_REQUEST_ENTITY_TOO_LARGE); } if (len > 0) { body.append(buffer, 0, len); diff --git a/java/org/apache/catalina/core/StandardWrapperValve.java b/java/org/apache/catalina/core/StandardWrapperValve.java index 70226ce3a9a5..1c3e5c31d060 100644 --- a/java/org/apache/catalina/core/StandardWrapperValve.java +++ b/java/org/apache/catalina/core/StandardWrapperValve.java @@ -38,6 +38,7 @@ 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; @@ -195,6 +196,13 @@ public void invoke(Request request, Response response) throws IOException, Servl } throwable = e; exception(request, response, e); + } catch (InvalidParameterException e) { + if (container.getLogger().isDebugEnabled()) { + container.getLogger().debug( + sm.getString("standardWrapper.serviceException", wrapper.getName(), context.getName()), e); + } + throwable = e; + exception(request, response, e, e.getErrorCode()); } catch (Throwable e) { ExceptionUtils.handleThrowable(e); container.getLogger() @@ -271,8 +279,12 @@ private void checkWrapperAvailable(Response response, StandardWrapper wrapper) t * @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); + } + + private void exception(Request request, Response response, Throwable exception, int errorCode) { request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, exception); - response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + response.setStatus(errorCode); 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 000000000000..f5162e11f623 --- /dev/null +++ b/java/org/apache/tomcat/util/http/InvalidParameterException.java @@ -0,0 +1,98 @@ +/* + * 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 jakarta.servlet.http.HttpServletResponse; + +/** + * Extend {@link IllegalStateException} to identify the cause as an invalid parameter. + *

+ * Implementation note: This class extends {@link IllegalStateException} since that is the class that the Servlet 6.1 + * onwards Javadocs define is thrown by the various {@code ServletRequest.getParameterXXX()} methods. + */ +public class InvalidParameterException extends IllegalStateException { + + private static final long serialVersionUID = 1L; + + private final int errorCode; + + + /** + * Construct a new exception with the given message. + * + * @param message The message to use for the exception + */ + public InvalidParameterException(String message) { + this(message, HttpServletResponse.SC_BAD_REQUEST); + } + + + /** + * Construct a new exception with the given message and error code. + * + * @param message The message to use for the exception + * @param errorCode The HTTP status code to use when reporting this error. Expected to be >= 400. + */ + public InvalidParameterException(String message, int errorCode) { + this(message, null, errorCode); + } + + + /** + * Construct a new exception with the given message and cause. + * + * @param message The message to use for the exception + * @param cause The exception to use as the cause of this exception + */ + public InvalidParameterException(String message, Throwable cause) { + this(message, cause, HttpServletResponse.SC_BAD_REQUEST); + } + + + /** + * Construct a new exception with the given cause. The message for this exception will be generated by calling + * {@code cause.toString()}. + * + * @param cause The exception to use as the cause of this exception + */ + public InvalidParameterException(Throwable cause) { + this(cause.toString(), cause, HttpServletResponse.SC_BAD_REQUEST); + } + + + /** + * Construct a new exception with the given cause and error code. The message for this exception will be generated + * by calling {@code cause.toString()}. + * + * @param cause The exception to use as the cause of this exception + * @param errorCode The HTTP status code to use when reporting this error. Expected to be >= 400. + */ + public InvalidParameterException(Throwable cause, int errorCode) { + this(cause.toString(), cause, errorCode); + } + + + private InvalidParameterException(String message, Throwable cause, int errorCode) { + super(message, cause); + this.errorCode = errorCode; + } + + + public int getErrorCode() { + return errorCode; + } +} \ No newline at end of file diff --git a/java/org/apache/tomcat/util/http/Parameters.java b/java/org/apache/tomcat/util/http/Parameters.java index b590d379e81f..4c517e83c45a 100644 --- a/java/org/apache/tomcat/util/http/Parameters.java +++ b/java/org/apache/tomcat/util/http/Parameters.java @@ -18,6 +18,7 @@ import java.io.IOException; import java.nio.charset.Charset; +import java.nio.charset.CodingErrorAction; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.Collections; @@ -31,17 +32,12 @@ import org.apache.tomcat.util.buf.MessageBytes; import org.apache.tomcat.util.buf.StringUtils; import org.apache.tomcat.util.buf.UDecoder; -import org.apache.tomcat.util.log.UserDataHelper; import org.apache.tomcat.util.res.StringManager; 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 maxParamCountLog = new UserDataHelper(log); - private static final StringManager sm = StringManager.getManager("org.apache.tomcat.util.http"); private final Map> paramHashValues = new LinkedHashMap<>(); @@ -202,10 +198,8 @@ public void addParameter(String key, String value) throws IllegalStateException } 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))); + // Processing this parameter will push us over the limit. + throw new InvalidParameterException(sm.getString("parameters.maxCountFail", Integer.valueOf(limit))); } parameterCount++; @@ -237,8 +231,6 @@ private void processParameters(byte bytes[], int start, int len, Charset charset log.debug(sm.getString("parameters.bytes", new String(bytes, start, len, DEFAULT_BODY_CHARSET))); } - int decodeFailCount = 0; - int pos = start; int end = start + len; @@ -316,30 +308,15 @@ private void processParameters(byte bytes[], int start, int len, Charset charset continue; } // &=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 message = sm.getString("parameters.invalidChunk", Integer.valueOf(nameStart), + Integer.valueOf(valueEnd), extract); + throw new InvalidParameterException(message); } tmpName.setBytes(bytes, nameStart, nameEnd - nameStart); @@ -374,89 +351,34 @@ private void processParameters(byte bytes[], int start, int len, Charset charset urlDecode(tmpName); } tmpName.setCharset(charset); - name = tmpName.toString(); + name = tmpName.toString(CodingErrorAction.REPORT, CodingErrorAction.REPORT); if (valueStart >= 0) { if (decodeValue) { urlDecode(tmpValue); } tmpValue.setCharset(charset); - value = tmpValue.toString(); + value = tmpValue.toString(CodingErrorAction.REPORT, CodingErrorAction.REPORT); } 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; - } + addParameter(name, value); } 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); - } - } - } + String message; + if (log.isDebugEnabled()) { + message = sm.getString("parameters.decodeFail.debug", origName.toString(), origValue.toString()); + } else { + message = sm.getString("parameters.decodeFail.info", tmpName.toString(), tmpValue.toString()); } - } - - 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 (logMode != null) { - String message = sm.getString("parameters.multipleDecodingFail", Integer.valueOf(decodeFailCount)); - switch (logMode) { - case INFO_THEN_DEBUG: - message += sm.getString("parameters.fallToDebug"); - //$FALL-THROUGH$ - case INFO: - log.info(message); - break; - case DEBUG: - log.debug(message); + throw new InvalidParameterException(message, e); + } finally { + tmpName.recycle(); + tmpValue.recycle(); + // Only recycle copies if we used them + if (log.isDebugEnabled()) { + origName.recycle(); + origValue.recycle(); } } } diff --git a/test/org/apache/tomcat/util/http/TestParameters.java b/test/org/apache/tomcat/util/http/TestParameters.java index 885b563fe21c..db1c07396512 100644 --- a/test/org/apache/tomcat/util/http/TestParameters.java +++ b/test/org/apache/tomcat/util/http/TestParameters.java @@ -40,7 +40,7 @@ public class TestParameters { new Parameter("\ufb6b\ufb6a\ufb72", "\uffee\uffeb\uffe2"); @Test - public void testProcessParametersByteArrayIntInt() { + public void testProcessParametersByteArrayIntIntValid() { doTestProcessParametersByteArrayIntInt(-1, SIMPLE); doTestProcessParametersByteArrayIntInt(-1, SIMPLE_MULTIPLE); doTestProcessParametersByteArrayIntInt(-1, NO_VALUE); @@ -60,14 +60,26 @@ public void testProcessParametersByteArrayIntInt() { doTestProcessParametersByteArrayIntInt(-1, UTF8, SIMPLE, SIMPLE_MULTIPLE, NO_VALUE, EMPTY_VALUE, EMPTY); + doTestProcessParametersByteArrayIntInt(4, + SIMPLE, NO_VALUE, EMPTY_VALUE, UTF8); + } + + @Test(expected = IllegalStateException.class) + public void testProcessParametersByteArrayIntIntInvalid01() { doTestProcessParametersByteArrayIntInt(1, SIMPLE, NO_VALUE, EMPTY_VALUE, UTF8); + } + + @Test(expected = IllegalStateException.class) + public void testProcessParametersByteArrayIntIntInvalid02() { doTestProcessParametersByteArrayIntInt(2, SIMPLE, NO_VALUE, EMPTY_VALUE, UTF8); + } + + @Test(expected = IllegalStateException.class) + public void testProcessParametersByteArrayIntIntInvalid03() { doTestProcessParametersByteArrayIntInt(3, SIMPLE, NO_VALUE, EMPTY_VALUE, UTF8); - doTestProcessParametersByteArrayIntInt(4, - SIMPLE, NO_VALUE, EMPTY_VALUE, UTF8); } // Make sure the inner Parameter class behaves correctly