From d93c5a7fd9213b2bc68774edf77c6d9f3b1c2620 Mon Sep 17 00:00:00 2001 From: Tomas Hofman Date: Wed, 27 Sep 2023 12:45:31 +0200 Subject: [PATCH 1/8] UNDERTOW-2309 Prevent memory leak in DefaultByteBufferPool --- .../server/DefaultByteBufferPool.java | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/io/undertow/server/DefaultByteBufferPool.java b/core/src/main/java/io/undertow/server/DefaultByteBufferPool.java index 0d8ea81961..811b0374e2 100644 --- a/core/src/main/java/io/undertow/server/DefaultByteBufferPool.java +++ b/core/src/main/java/io/undertow/server/DefaultByteBufferPool.java @@ -26,7 +26,9 @@ import java.nio.ByteBuffer; import java.util.ArrayDeque; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicIntegerFieldUpdater; @@ -39,7 +41,7 @@ */ public class DefaultByteBufferPool implements ByteBufferPool { - private final ThreadLocal threadLocalCache = new ThreadLocal<>(); + private final ThreadLocalCache threadLocalCache = new ThreadLocalCache(); // Access requires synchronization on the threadLocalDataList instance private final List> threadLocalDataList = new ArrayList<>(); private final ConcurrentLinkedQueue queue = new ConcurrentLinkedQueue<>(); @@ -229,6 +231,7 @@ public void close() { local.buffers.clear(); } ref.clear(); + threadLocalCache.remove(local); } threadLocalDataList.clear(); } @@ -327,4 +330,29 @@ protected void finalize() throws Throwable { } } + // This is used instead of Java ThreadLocal class. Unlike in the ThreadLocal class, the remove() method in this + // class can be called by a different thread than the one that initialized the data. + private static class ThreadLocalCache { + + Map localsByThread = new HashMap<>(); + + ThreadLocalData get() { + return localsByThread.get(Thread.currentThread()); + } + + void set(ThreadLocalData threadLocalData) { + localsByThread.put(Thread.currentThread(), threadLocalData); + } + + void remove(ThreadLocalData threadLocalData) { + // Find the entry containing given data instance and remove it from the map. + for (Map.Entry entry: localsByThread.entrySet()) { + if (threadLocalData.equals(entry.getValue())) { + localsByThread.remove(entry.getKey(), entry.getValue()); + break; + } + } + } + } + } From b8f535d5003ee9eae70a4be4c6f91ce9c460561b Mon Sep 17 00:00:00 2001 From: Flavia Rainone Date: Sun, 22 Oct 2023 04:22:13 -0300 Subject: [PATCH 2/8] [UNDERTOW-2330] At HttpReadListener, handle race when connection is shutdown concurrently with buffer allocation Signed-off-by: Flavia Rainone --- .../server/protocol/http/HttpReadListener.java | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/undertow/server/protocol/http/HttpReadListener.java b/core/src/main/java/io/undertow/server/protocol/http/HttpReadListener.java index 53a737ab7d..c79aa19b83 100644 --- a/core/src/main/java/io/undertow/server/protocol/http/HttpReadListener.java +++ b/core/src/main/java/io/undertow/server/protocol/http/HttpReadListener.java @@ -139,12 +139,21 @@ public void handleEvent(final ConduitStreamSourceChannel channel) { public void handleEventWithNoRunningRequest(final ConduitStreamSourceChannel channel) { PooledByteBuffer existing = connection.getExtraBytes(); if ((existing == null && connection.getOriginalSourceConduit().isReadShutdown()) || connection.getOriginalSinkConduit().isWriteShutdown()) { + UndertowLogger.REQUEST_IO_LOGGER.debug("Connection is closing, cancelling handling of request"); + IoUtils.safeClose(connection); + channel.suspendReads(); + return; + } + final PooledByteBuffer pooled; + try { + pooled = existing == null ? connection.getByteBufferPool().allocate() : existing; + } catch (IllegalStateException e) { + UndertowLogger.REQUEST_IO_LOGGER.debug("Connection is closing, cancelling handling of request", e); + // shutdown started after previous if statement, so treat it like previous statement IoUtils.safeClose(connection); channel.suspendReads(); return; } - - final PooledByteBuffer pooled = existing == null ? connection.getByteBufferPool().allocate() : existing; final ByteBuffer buffer = pooled.getBuffer(); boolean free = true; From 53cfa2f3fd8089c805e316cee1498c8364514e60 Mon Sep 17 00:00:00 2001 From: Flavia Rainone Date: Mon, 23 Oct 2023 02:00:50 -0300 Subject: [PATCH 3/8] [UNDERTOW-2331] At RapidResetDDoSUnitTestCase, accept a smaller number of responses than expected as the result of race condition where client notices the connection is closed by server before processing responses and it marks reads as broken Relates to UNDERTOW-2328 Signed-off-by: Flavia Rainone --- .../io/undertow/protocols/http2/RapidResetDDoSUnitTestCase.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/io/undertow/protocols/http2/RapidResetDDoSUnitTestCase.java b/core/src/test/java/io/undertow/protocols/http2/RapidResetDDoSUnitTestCase.java index 95191779a5..08e5e7b5aa 100644 --- a/core/src/test/java/io/undertow/protocols/http2/RapidResetDDoSUnitTestCase.java +++ b/core/src/test/java/io/undertow/protocols/http2/RapidResetDDoSUnitTestCase.java @@ -243,7 +243,7 @@ public void assertDoSRstFramesHandled(int totalNumberOfRequests, int rstStreamLi // server sent go away before processing and responding client frames, sometimes this happens, depends on the order of threads // being executed - if (responses.isEmpty()) { + if (responses.size() < totalNumberOfRequests) { Assert.assertTrue(errorExpected); Assert.assertNotNull(exception); Assert.assertTrue(exception instanceof ClosedChannelException); From 34a4f5ab62a781460801eb5436ca3e03e9481ef9 Mon Sep 17 00:00:00 2001 From: Lin Gao Date: Wed, 20 Dec 2023 12:31:00 +0800 Subject: [PATCH 4/8] [UNDERTOW-2337] Multipart form-data larger than 16KiB is not available through Servlet getParameter API --- .../handlers/RequestDumpingHandler.java | 2 +- .../server/handlers/form/FormData.java | 37 ++++++++++++-- .../form/MultiPartParserDefinition.java | 27 ++++++----- .../main/java/io/undertow/util/FileUtils.java | 20 +++++++- .../form/MultipartFormDataParserTestCase.java | 3 +- .../servlet/spec/HttpServletRequestImpl.java | 10 ++-- .../multipart/AddMultipartServetListener.java | 4 ++ .../test/multipart/MultiPartServlet.java | 19 ++++++++ .../test/multipart/MultiPartTestCase.java | 48 +++++++++++++++++++ 9 files changed, 145 insertions(+), 25 deletions(-) diff --git a/core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java b/core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java index 6b97ab8db4..48282ae5b6 100644 --- a/core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java +++ b/core/src/main/java/io/undertow/server/handlers/RequestDumpingHandler.java @@ -164,7 +164,7 @@ private void dumpRequestBody(HttpServerExchange exchange, StringBuilder sb) { sb.append(formField) .append("="); for (FormData.FormValue formValue : formValues) { - sb.append(formValue.isFileItem() ? "[file-content]" : formValue.getValue()); + sb.append(formValue.isFileItem() && !formValue.isBigField() ? "[file-content]" : formValue.getValue()); sb.append("\n"); if (formValue.getHeaders() != null) { diff --git a/core/src/main/java/io/undertow/server/handlers/form/FormData.java b/core/src/main/java/io/undertow/server/handlers/form/FormData.java index d7b5689b66..b4d9eef053 100644 --- a/core/src/main/java/io/undertow/server/handlers/form/FormData.java +++ b/core/src/main/java/io/undertow/server/handlers/form/FormData.java @@ -34,6 +34,7 @@ import java.util.Map; import io.undertow.UndertowMessages; +import io.undertow.util.FileUtils; import io.undertow.util.HeaderMap; /** @@ -102,11 +103,15 @@ public void add(String name, String value, String charset, final HeaderMap heade } public void add(String name, Path value, String fileName, final HeaderMap headers) { + add(name, value, fileName, headers, false, null); + } + + public void add(String name, Path value, String fileName, final HeaderMap headers, boolean bigField, String charset) { Deque values = this.values.get(name); if (values == null) { this.values.put(name, values = new ArrayDeque<>(1)); } - values.add(new FormValueImpl(value, fileName, headers)); + values.add(new FormValueImpl(value, fileName, headers, bigField, charset)); if (values.size() > maxValues) { throw new RuntimeException(UndertowMessages.MESSAGES.tooManyParameters(maxValues)); } @@ -211,6 +216,16 @@ public interface FormValue { * @return The headers that were present in the multipart request, or null if this was not a multipart request */ HeaderMap getHeaders(); + + /** + * @return true if size of the FormValue comes from a multipart request exceeds the fieldSizeThreshold of + * {@link MultiPartParserDefinition} without filename specified. + * + * A big field is stored in disk, it is a file based FileItem. + * + * getValue() returns getCharset() decoded string from the file if it is true. + */ + boolean isBigField(); } public static class FileItem { @@ -284,6 +299,7 @@ static class FormValueImpl implements FormValue { private final HeaderMap headers; private final FileItem fileItem; private final String charset; + private final boolean bigField; FormValueImpl(String value, HeaderMap headers) { this.value = value; @@ -291,6 +307,7 @@ static class FormValueImpl implements FormValue { this.fileName = null; this.fileItem = null; this.charset = null; + this.bigField = false; } FormValueImpl(String value, String charset, HeaderMap headers) { @@ -299,14 +316,16 @@ static class FormValueImpl implements FormValue { this.headers = headers; this.fileName = null; this.fileItem = null; + this.bigField = false; } - FormValueImpl(Path file, final String fileName, HeaderMap headers) { + FormValueImpl(Path file, final String fileName, HeaderMap headers, boolean bigField, String charset) { this.fileItem = new FileItem(file); this.headers = headers; this.fileName = fileName; this.value = null; - this.charset = null; + this.charset = charset; + this.bigField = bigField; } FormValueImpl(byte[] data, String fileName, HeaderMap headers) { @@ -315,12 +334,20 @@ static class FormValueImpl implements FormValue { this.headers = headers; this.value = null; this.charset = null; + this.bigField = false; } @Override public String getValue() { if (value == null) { + if (bigField) { + try (InputStream inputStream = getFileItem().getInputStream()) { + return FileUtils.readFile(inputStream, this.charset); + } catch (IOException e) { + // ignore + } + } throw UndertowMessages.MESSAGES.formValueIsAFile(); } return value; @@ -360,6 +387,10 @@ public FileItem getFileItem() { return fileItem; } + public boolean isBigField() { + return bigField; + } + @Override public boolean isFileItem() { return fileItem != null; diff --git a/core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java b/core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java index e555280633..bba234a47b 100644 --- a/core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java +++ b/core/src/main/java/io/undertow/server/handlers/form/MultiPartParserDefinition.java @@ -350,8 +350,7 @@ public void endPart() { if (fileName != null) { data.add(currentName, file, fileName, headers); } else { - final Path fileNamePath = file.getFileName(); - data.add(currentName, file, fileNamePath != null ? fileNamePath.toString() : "", headers); + data.add(currentName, file, null, headers, true, getCharset()); } file = null; contentBytes.reset(); @@ -365,18 +364,8 @@ public void endPart() { data.add(currentName, Arrays.copyOf(contentBytes.toByteArray(), contentBytes.size()), fileName, headers); contentBytes.reset(); } else { - - try { - String charset = defaultEncoding; - String contentType = headers.getFirst(Headers.CONTENT_TYPE); - if (contentType != null) { - String cs = Headers.extractQuotedValueFromHeader(contentType, "charset"); - if (cs != null) { - charset = cs; - } - } - + String charset = getCharset(); data.add(currentName, new String(contentBytes.toByteArray(), charset), charset, headers); } catch (UnsupportedEncodingException e) { throw new RuntimeException(e); @@ -385,6 +374,18 @@ public void endPart() { } } + private String getCharset() { + String charset = defaultEncoding; + String contentType = headers.getFirst(Headers.CONTENT_TYPE); + if (contentType != null) { + String cs = Headers.extractQuotedValueFromHeader(contentType, "charset"); + if (cs != null) { + charset = cs; + } + } + return charset; + } + public List getCreatedFiles() { return createdFiles; } diff --git a/core/src/main/java/io/undertow/util/FileUtils.java b/core/src/main/java/io/undertow/util/FileUtils.java index ad34a45b35..f47d3448c3 100644 --- a/core/src/main/java/io/undertow/util/FileUtils.java +++ b/core/src/main/java/io/undertow/util/FileUtils.java @@ -22,7 +22,9 @@ import java.io.IOException; import java.io.InputStream; import java.net.URL; +import java.nio.charset.Charset; import java.nio.charset.StandardCharsets; +import java.nio.charset.UnsupportedCharsetException; import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.Path; @@ -51,16 +53,32 @@ public static String readFile(URL url) { } } + public static String readFile(InputStream file, String charset) { + try { + Charset charSet = charset != null ? Charset.forName(charset) : StandardCharsets.UTF_8; + return readFile(file, charSet); + } catch (UnsupportedCharsetException e) { + throw new RuntimeException(e); + } + } + /** * Reads the {@link InputStream file} and converting it to {@link String} using UTF-8 encoding. */ public static String readFile(InputStream file) { + return readFile(file, StandardCharsets.UTF_8); + } + + /** + * Reads the {@link InputStream file} and converting it to {@link String} using charSet encoding. + */ + public static String readFile(InputStream file, Charset charSet) { try (BufferedInputStream stream = new BufferedInputStream(file)) { byte[] buff = new byte[1024]; StringBuilder builder = new StringBuilder(); int read; while ((read = stream.read(buff)) != -1) { - builder.append(new String(buff, 0, read, StandardCharsets.UTF_8)); + builder.append(new String(buff, 0, read, charSet)); } return builder.toString(); } catch (IOException e) { diff --git a/core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java b/core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java index 7bd7eddebc..69c8090368 100644 --- a/core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java +++ b/core/src/test/java/io/undertow/server/handlers/form/MultipartFormDataParserTestCase.java @@ -370,8 +370,7 @@ public void testLargeContentWithoutFileNameWithSmallFileSizeThreshold() throws E Assert.assertEquals("false", parsedResponse.get("in_memory")); Assert.assertEquals(DigestUtils.md5Hex(Files.newInputStream(file.toPath())), parsedResponse.get("hash")); - Assert.assertTrue(parsedResponse.get("file_name").startsWith("undertow")); - Assert.assertTrue(parsedResponse.get("file_name").endsWith("upload")); + Assert.assertEquals(parsedResponse.get("file_name"), "null"); } finally { if (!file.delete()) { file.deleteOnExit(); diff --git a/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java b/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java index a1992c1df3..b0332be0a9 100644 --- a/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java +++ b/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java @@ -738,7 +738,7 @@ public String getParameter(final String name) { final FormData parsedFormData = parseFormData(); if (parsedFormData != null) { FormData.FormValue res = parsedFormData.getFirst(name); - if (res == null || res.isFileItem()) { + if (res == null || res.isFileItem() && !res.isBigField()) { return null; } else { return res.getValue(); @@ -761,7 +761,7 @@ public Enumeration getParameterNames() { while (it.hasNext()) { String name = it.next(); for(FormData.FormValue param : parsedFormData.get(name)) { - if(!param.isFileItem()) { + if(!param.isFileItem() || param.isBigField()) { parameterNames.add(name); break; } @@ -788,7 +788,7 @@ public String[] getParameterValues(final String name) { Deque res = parsedFormData.get(name); if (res != null) { for (FormData.FormValue value : res) { - if(!value.isFileItem()) { + if(!value.isFileItem() || value.isBigField()) { ret.add(value.getValue()); } } @@ -819,14 +819,14 @@ public Map getParameterMap() { if (arrayMap.containsKey(name)) { ArrayList existing = arrayMap.get(name); for (final FormData.FormValue v : val) { - if(!v.isFileItem()) { + if(!v.isFileItem() || v.isBigField()) { existing.add(v.getValue()); } } } else { final ArrayList values = new ArrayList<>(); for (final FormData.FormValue v : val) { - if(!v.isFileItem()) { + if(!v.isFileItem() || v.isBigField()) { values.add(v.getValue()); } } diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/AddMultipartServetListener.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/AddMultipartServetListener.java index 6c8a8b6913..44f8e5763a 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/AddMultipartServetListener.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/AddMultipartServetListener.java @@ -14,6 +14,10 @@ public void contextInitialized(ServletContextEvent sce) { ServletRegistration.Dynamic reg = sce.getServletContext().addServlet("added", new MultiPartServlet()); reg.addMapping("/added"); reg.setMultipartConfig(new MultipartConfigElement(System.getProperty("java.io.tmpdir"))); + + reg = sce.getServletContext().addServlet("getParam", new MultiPartServlet(true)); + reg.addMapping("/getParam"); + reg.setMultipartConfig(new MultipartConfigElement(System.getProperty("java.io.tmpdir"))); } @Override diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java index 81aabc32b2..bb944dd038 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java @@ -37,6 +37,17 @@ */ public class MultiPartServlet extends HttpServlet { + private final boolean getParam; + public MultiPartServlet() { + super(); + this.getParam = false; + } + + public MultiPartServlet(boolean getParam) { + super(); + this.getParam = getParam; + } + @Override protected void doPost(final HttpServletRequest req, final HttpServletResponse resp) throws ServletException, IOException { try { @@ -58,6 +69,14 @@ protected void doPost(final HttpServletRequest req, final HttpServletResponse re writer.println("size: " + part.getSize()); writer.println("content: " + FileUtils.readFile(part.getInputStream())); } + if (getParam) { + Enumeration paramNames = req.getParameterNames(); + while (paramNames.hasMoreElements()) { + String name = paramNames.nextElement(); + writer.println("param name: " + name); + writer.println("param value: " + req.getParameter(name)); + } + } } catch (Exception e) { resp.getWriter().write("EXCEPTION: " + e.getClass()); } diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java index d7e1c3d8ba..ec62fd5add 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java @@ -251,4 +251,52 @@ public void testMultiPartRequestUtf8CharsetInPart() throws IOException { } } + @Test + public void testMultiPartRequestBigPostForm() throws IOException { + TestHttpClient client = new TestHttpClient(); + try { + String uri = DefaultServer.getDefaultServerURL() + "/servletContext/getParam"; + HttpPost post = new HttpPost(uri); + + MultipartEntity entity = new MultipartEntity(HttpMultipartMode.BROWSER_COMPATIBLE, null, StandardCharsets.UTF_8); + + String myValue = generateContent("myValue", 0x4000 * 2); + entity.addPart("formValue", new StringBody(myValue, "text/plain", StandardCharsets.UTF_8)); + entity.addPart("file", new FileBody(new File(MultiPartTestCase.class.getResource("uploadfile.txt").getFile()))); + + post.setEntity(entity); + HttpResponse result = client.execute(post); + Assert.assertEquals(StatusCodes.OK, result.getStatusLine().getStatusCode()); + final String response = HttpClientUtils.readResponse(result); + Assert.assertEquals("PARAMS:\r\n" + + "parameter count: 1\r\n" + + "parameter name count: 1\r\n" + + "name: formValue\r\n" + + "filename: null\r\n" + + "content-type: null\r\n" + + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "size: " + myValue.getBytes(StandardCharsets.UTF_8).length + "\r\n" + + "content: " + myValue + "\r\n" + + "name: file\r\n" + + "filename: uploadfile.txt\r\n" + + "content-type: application/octet-stream\r\n" + + "Content-Disposition: form-data; name=\"file\"; filename=\"uploadfile.txt\"\r\n" + + "Content-Type: application/octet-stream\r\n" + + "size: 13\r\n" + + "content: file contents\r\n" + + "param name: formValue\r\n" + + "param value: " + myValue + "\r\n", response); + } finally { + client.getConnectionManager().shutdown(); + } + } + + private String generateContent(String chunk, int size) { + int checkLength = chunk.getBytes().length; + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < size / checkLength; i++) { + sb.append(chunk); + } + return sb.toString(); + } } From 419e855a210114232d475d18346ee606b9bd777f Mon Sep 17 00:00:00 2001 From: Flavia Rainone Date: Mon, 12 Feb 2024 16:50:06 -0300 Subject: [PATCH 5/8] [UNDERTOW-2337] Also verify if the value of the part can be retrieved as a parameter Signed-off-by: Flavia Rainone --- .../servlet/test/multipart/MultiPartServlet.java | 12 ++++++++++++ .../servlet/test/multipart/MultiPartTestCase.java | 4 ++++ 2 files changed, 16 insertions(+) diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java index bb944dd038..c4345c99d2 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartServlet.java @@ -30,6 +30,7 @@ import javax.servlet.http.HttpServletResponse; import javax.servlet.http.Part; +import io.undertow.util.Headers; import io.undertow.util.FileUtils; /** @@ -65,6 +66,17 @@ protected void doPost(final HttpServletRequest req, final HttpServletResponse re Collection headerNames = new TreeSet<>(part.getHeaderNames()); for (String header : headerNames) { writer.println(header + ": " + part.getHeader(header)); + if (header.equals(Headers.CONTENT_DISPOSITION_STRING)) { + final String parameterValue = part.getHeader(header); + String parameterName = parameterValue.substring(parameterValue.indexOf("name=") + "name=\"".length()); + parameterName = parameterName.substring(0, parameterName.indexOf('\"')); + final String[] values = req.getParameterValues(parameterName); + if (values != null) { + for (String value: values) { + writer.println("value: " + value); + } + } + } } writer.println("size: " + part.getSize()); writer.println("content: " + FileUtils.readFile(part.getInputStream())); diff --git a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java index ec62fd5add..93d7547dc6 100644 --- a/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java +++ b/servlet/src/test/java/io/undertow/servlet/test/multipart/MultiPartTestCase.java @@ -126,6 +126,7 @@ public void testMultiPartRequest() throws IOException { "filename: null\r\n" + "content-type: null\r\n" + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "value: myValue\r\n" + "size: 7\r\n" + "content: myValue\r\n" + "name: file\r\n" + @@ -163,6 +164,7 @@ public void testMultiPartRequestWithAddedServlet() throws IOException { "filename: null\r\n" + "content-type: null\r\n" + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "value: myValue\r\n" + "size: 7\r\n" + "content: myValue\r\n" + "name: file\r\n" + @@ -242,6 +244,7 @@ public void testMultiPartRequestUtf8CharsetInPart() throws IOException { "filename: null\r\n" + "content-type: text/plain; charset=UTF-8\r\n" + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "value: " + "myValue" + '\u00E5' + "\r\n" + "Content-Transfer-Encoding: 8bit\r\n" + "Content-Type: text/plain; charset=UTF-8\r\n" + "size: 9\r\n" + @@ -275,6 +278,7 @@ public void testMultiPartRequestBigPostForm() throws IOException { "filename: null\r\n" + "content-type: null\r\n" + "Content-Disposition: form-data; name=\"formValue\"\r\n" + + "value: " + myValue + "\r\n" + "size: " + myValue.getBytes(StandardCharsets.UTF_8).length + "\r\n" + "content: " + myValue + "\r\n" + "name: file\r\n" + From ea7ea2525d9acad0fcf8f3dfd4972f91394796ee Mon Sep 17 00:00:00 2001 From: baranowb Date: Mon, 5 Feb 2024 08:58:22 +0100 Subject: [PATCH 6/8] [UNDERTOW-2342] CVE-2023-4639 ignore cookie with improper quotes Signed-off-by: Flavia Rainone --- core/src/main/java/io/undertow/util/Cookies.java | 3 +++ .../java/io/undertow/util/CookiesTestCase.java | 15 +++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/core/src/main/java/io/undertow/util/Cookies.java b/core/src/main/java/io/undertow/util/Cookies.java index f84ad5315c..d69dd7327a 100644 --- a/core/src/main/java/io/undertow/util/Cookies.java +++ b/core/src/main/java/io/undertow/util/Cookies.java @@ -316,6 +316,9 @@ private static void parseCookie(final String cookie, final Set parsedCoo cookieCount = createCookie(name, containsEscapedQuotes ? unescapeDoubleQuotes(cookie.substring(start, i)) : cookie.substring(start, i), maxCookies, cookieCount, cookies, additional); state = 0; start = i + 1; + } else if (c == ';' || (commaIsSeperator && c == ',')) { + state = 0; + start = i + 1; } // Skip the next double quote char '"' when it is escaped by backslash '\' (i.e. \") inside the quoted value if (c == '\\' && (i + 1 < cookie.length()) && cookie.charAt(i + 1) == '"') { diff --git a/core/src/test/java/io/undertow/util/CookiesTestCase.java b/core/src/test/java/io/undertow/util/CookiesTestCase.java index 7284f2e8a4..f0d276146d 100644 --- a/core/src/test/java/io/undertow/util/CookiesTestCase.java +++ b/core/src/test/java/io/undertow/util/CookiesTestCase.java @@ -449,6 +449,21 @@ public void testSameSiteCookie() { Assert.assertNull(cookie.getSameSiteMode()); } + @Test + public void testNoDoubleQuoteTermination() { + Map cookies = Cookies.parseRequestCookies(4, false, Arrays.asList("CUSTOMER=\"WILE_E_COYOTE\"; BAD=\"X; SHIPPING=FEDEX"), true); + Assert.assertEquals(2, cookies.size()); + Cookie cookie = cookies.get("CUSTOMER"); + Assert.assertEquals("CUSTOMER", cookie.getName()); + Assert.assertEquals("WILE_E_COYOTE", cookie.getValue()); + cookie = cookies.get("BAD"); + Assert.assertNull(cookie); + cookie = cookies.get("SHIPPING"); + Assert.assertEquals("SHIPPING", cookie.getName()); + Assert.assertEquals("FEDEX", cookie.getValue()); + Assert.assertNotNull(cookie); + } + // RFC6265 allows US-ASCII characters excluding CTLs, whitespace, // double quote, comma, semicolon and backslash as cookie value. // This does not change even if value is quoted. From 2e99d958f52141b0b391c47b7e4a6c94be10704b Mon Sep 17 00:00:00 2001 From: Flavia Rainone Date: Tue, 13 Feb 2024 04:27:55 -0300 Subject: [PATCH 7/8] [UNDERTOW-2304] At SslConduit, only delegate handshake task if the task is going to run on a different set of threads than current thread Signed-off-by: Flavia Rainone --- .../io/undertow/protocols/ssl/SslConduit.java | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/io/undertow/protocols/ssl/SslConduit.java b/core/src/main/java/io/undertow/protocols/ssl/SslConduit.java index 91b6c898d7..3a2068aceb 100644 --- a/core/src/main/java/io/undertow/protocols/ssl/SslConduit.java +++ b/core/src/main/java/io/undertow/protocols/ssl/SslConduit.java @@ -206,7 +206,9 @@ public void run() { this.sink = delegate.getSinkChannel().getConduit(); this.source = delegate.getSourceChannel().getConduit(); this.engine = engine; - this.delegatedTaskExecutor = delegatedTaskExecutor; + // if delegatedTaskExecutor is the same as delegate.getWorker, don't set it (see handshake task execution below, + // we need to pick a thread that is different from the current thread running or run the handshake task immediately at the same thread) + this.delegatedTaskExecutor = this.delegate.getWorker() != delegatedTaskExecutor? delegatedTaskExecutor : null; this.bufferPool = bufferPool; delegate.getSourceChannel().getConduit().setReadReadyHandler(readReadyHandler = new SslReadReadyHandler(null)); delegate.getSinkChannel().getConduit().setWriteReadyHandler(writeReadyHandler = new SslWriteReadyHandler(null)); @@ -1168,7 +1170,16 @@ public void run() { } }; try { - getDelegatedTaskExecutor().execute(wrappedTask); + // we want to pick a thread different from the one we are running, to prevent starvation scenarios + // for example where we repeatedly attempt to write or read but are stuck waiting on a handshake task + // and the handshake task rarely has the chance to run because it needs the read/write thread to complete execution + // so, if io thread (read and write thread) is the same as current thread, and getDelegatedTaskExecutor will delegate + // execution to the same set of threads we are currently at, just run the handshake task right away to prevent blocking + if (delegate.getIoThread().equals(Thread.currentThread()) && delegatedTaskExecutor == null) { + wrappedTask.run(); + } else { + getDelegatedTaskExecutor().execute(wrappedTask); + } } catch (RejectedExecutionException e) { UndertowLogger.REQUEST_IO_LOGGER.sslEngineDelegatedTaskRejected(e); IoUtils.safeClose(connection); From 34724c42e45223f781a1e8f3a8c9207ed6191bcb Mon Sep 17 00:00:00 2001 From: Flavia Rainone Date: Tue, 13 Feb 2024 06:59:03 -0300 Subject: [PATCH 8/8] [UNDERTOW-2338] Fix NPE by making sure asyncStart is true only if asyncContext is initialized Signed-off-by: Flavia Rainone --- .../undertow/servlet/spec/HttpServletRequestImpl.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java b/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java index b0332be0a9..dcd3781f66 100644 --- a/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java +++ b/servlet/src/main/java/io/undertow/servlet/spec/HttpServletRequestImpl.java @@ -1060,9 +1060,10 @@ public AsyncContext startAsync() throws IllegalStateException { } else if (asyncStarted) { throw UndertowServletMessages.MESSAGES.asyncAlreadyStarted(); } - asyncStarted = true; final ServletRequestContext servletRequestContext = exchange.getAttachment(ServletRequestContext.ATTACHMENT_KEY); - return asyncContext = new AsyncContextImpl(exchange, servletRequestContext.getServletRequest(), servletRequestContext.getServletResponse(), servletRequestContext, false, asyncContext); + asyncContext = new AsyncContextImpl(exchange, servletRequestContext.getServletRequest(), servletRequestContext.getServletResponse(), servletRequestContext, false, asyncContext); + asyncStarted = true; + return asyncContext; } @Override @@ -1085,10 +1086,11 @@ public AsyncContext startAsync(final ServletRequest servletRequest, final Servle } else if (asyncStarted) { throw UndertowServletMessages.MESSAGES.asyncAlreadyStarted(); } - asyncStarted = true; servletRequestContext.setServletRequest(servletRequest); servletRequestContext.setServletResponse(servletResponse); - return asyncContext = new AsyncContextImpl(exchange, servletRequest, servletResponse, servletRequestContext, true, asyncContext); + asyncContext = new AsyncContextImpl(exchange, servletRequest, servletResponse, servletRequestContext, true, asyncContext); + asyncStarted = true; + return asyncContext; } @Override