From 7488452e8a95bca73a0e69694c34c5cac8c64661 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 3 Dec 2012 14:55:22 -0700 Subject: [PATCH 1/4] Adding some debugging --- .../src/main/java/org/eclipse/jetty/start/Main.java | 12 ++++++++---- .../main/java/org/eclipse/jetty/start/Monitor.java | 3 +-- .../src/main/java/com/acme/CookieDump.java | 1 + 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java index 8323e53852f..b066a569145 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java @@ -1040,11 +1040,15 @@ public class Main if (timeout > 0) { - System.err.println("Waiting"+(timeout > 0 ? (" "+timeout+"sec") : "")+" for jetty to stop"); + System.err.printf("Waiting %,d seconds for jetty to stop%n", timeout); LineNumberReader lin = new LineNumberReader(new InputStreamReader(s.getInputStream())); - String response=lin.readLine(); - if ("Stopped".equals(response)) - System.err.println("Stopped"); + String response; + while ((response = lin.readLine()) != null) + { + Config.debug("Received \"" + response + "\""); + if ("Stopped".equals(response)) + System.err.println("Server reports itself as Stopped"); + } } } finally diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Monitor.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Monitor.java index ff29f11df94..1f5667b09af 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Monitor.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Monitor.java @@ -17,7 +17,6 @@ // package org.eclipse.jetty.start; -import java.io.IOException; import java.io.InputStreamReader; import java.io.LineNumberReader; import java.net.InetAddress; @@ -114,6 +113,7 @@ public class Monitor extends Thread Config.debug("command=" + cmd); if ("stop".equals(cmd)) { + Config.debug("Child Process: " + _process); if (_process!=null) { //if we have a child process, wait for it to finish before we stop @@ -121,7 +121,6 @@ public class Monitor extends Thread { _process.destroy(); _process.waitFor(); - } catch (InterruptedException e) { diff --git a/test-jetty-webapp/src/main/java/com/acme/CookieDump.java b/test-jetty-webapp/src/main/java/com/acme/CookieDump.java index 46145898ecc..d5d32e3a9ce 100644 --- a/test-jetty-webapp/src/main/java/com/acme/CookieDump.java +++ b/test-jetty-webapp/src/main/java/com/acme/CookieDump.java @@ -128,6 +128,7 @@ public class CookieDump extends HttpServlet // For testing --stop with STOP.WAIT handling of the jetty-start behavior. if (Boolean.getBoolean("test.slow.destroy")) { + log("Simulating a slow destroy (10 seconds)",null); try { TimeUnit.SECONDS.sleep(10); From 7abd512b32df732f185c2a2e0752a70575df43f1 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 4 Dec 2012 10:18:54 -0700 Subject: [PATCH 2/4] 391623 - Making --stop with STOP.WAIT perform graceful shutdown + Moving jetty-start Monitor to jetty-server ShutdownMonitor and using ShutdownThread to perform a graceful shutdown instead. --- .../java/org/eclipse/jetty/server/Server.java | 6 +- .../eclipse/jetty/server/ShutdownMonitor.java | 235 ++++++++++++++++++ .../java/org/eclipse/jetty/start/Main.java | 83 +++++-- .../java/org/eclipse/jetty/start/Monitor.java | 157 ------------ 4 files changed, 296 insertions(+), 185 deletions(-) create mode 100644 jetty-server/src/main/java/org/eclipse/jetty/server/ShutdownMonitor.java delete mode 100644 jetty-start/src/main/java/org/eclipse/jetty/start/Monitor.java diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index c891d63066d..9fc62e9f57d 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -21,6 +21,7 @@ package org.eclipse.jetty.server; import java.io.IOException; import java.net.InetSocketAddress; import java.util.Enumeration; + import javax.servlet.ServletException; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; @@ -260,11 +261,14 @@ public class Server extends HandlerWrapper implements Attributes @Override protected void doStart() throws Exception { - if (getStopAtShutdown()) + if (getStopAtShutdown()) { ShutdownThread.register(this); + ShutdownMonitor.getInstance(); // initialize + } LOG.info("jetty-"+__version); HttpGenerator.setServerVersion(__version); + MultiException mex=new MultiException(); if (_threadPool==null) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ShutdownMonitor.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ShutdownMonitor.java new file mode 100644 index 00000000000..25335475957 --- /dev/null +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ShutdownMonitor.java @@ -0,0 +1,235 @@ +// +// ======================================================================== +// Copyright (c) 1995-2012 Mort Bay Consulting Pty. Ltd. +// ------------------------------------------------------------------------ +// All rights reserved. This program and the accompanying materials +// are made available under the terms of the Eclipse Public License v1.0 +// and Apache License v2.0 which accompanies this distribution. +// +// The Eclipse Public License is available at +// http://www.eclipse.org/legal/epl-v10.html +// +// The Apache License v2.0 is available at +// http://www.opensource.org/licenses/apache2.0.php +// +// You may elect to redistribute this code under either of these licenses. +// ======================================================================== +// + +package org.eclipse.jetty.server; + +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.LineNumberReader; +import java.io.OutputStream; +import java.net.InetAddress; +import java.net.ServerSocket; +import java.net.Socket; +import java.util.Properties; + +import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.util.thread.ShutdownThread; + +/** + * Shutdown/Stop Monitor thread. + *

+ * This thread listens on the port specified by the STOP.PORT system parameter (defaults to -1 for not listening) for + * request authenticated with the key given by the STOP.KEY system parameter (defaults to "eclipse") for admin requests. + *

+ * If the stop port is set to zero, then a random port is assigned and the port number is printed to stdout. + *

+ * Commands "stop" and "status" are currently supported. + */ +public class ShutdownMonitor extends Thread +{ + private static final ShutdownMonitor INSTANCE = new ShutdownMonitor(); + + public static ShutdownMonitor getInstance() + { + return INSTANCE; + } + + private final boolean DEBUG; + private final int port; + private final String key; + private final ServerSocket serverSocket; + + private ShutdownMonitor() + { + Properties props = System.getProperties(); + + // Use the same debug option as /jetty-start/ + this.DEBUG = props.containsKey("DEBUG"); + + // Use values passed thru /jetty-start/ + int stopPort = Integer.parseInt(props.getProperty("STOP.PORT","-1")); + String stopKey = props.getProperty("STOP.KEY",null); + + ServerSocket sock = null; + + try + { + if (stopPort < 0) + { + System.out.println("ShutdownMonitor not in use"); + sock = null; + return; + } + + setDaemon(true); + setName("ShutdownMonitor"); + + sock = new ServerSocket(stopPort,1,InetAddress.getByName("127.0.0.1")); + if (stopPort == 0) + { + // server assigned port in use + stopPort = sock.getLocalPort(); + System.out.printf("STOP.PORT=%d%n",stopPort); + } + + if (stopKey == null) + { + // create random key + stopKey = Long.toString((long)(Long.MAX_VALUE * Math.random() + this.hashCode() + System.currentTimeMillis()),36); + System.out.printf("STOP.KEY=%s%n",stopKey); + } + + } + catch (Exception e) + { + debug(e); + System.err.println("Error binding monitor port " + stopPort + ": " + e.toString()); + } + finally + { + // establish the port and key that are in use + this.port = stopPort; + this.key = stopKey; + + this.serverSocket = sock; + debug("STOP.PORT=%d", port); + debug("STOP.KEY=%s", key); + debug("%s", serverSocket); + } + + this.start(); + } + + @Override + public String toString() + { + return String.format("%s[port=%d]",this.getClass().getName(),port); + } + + private void debug(Throwable t) + { + if (DEBUG) + { + t.printStackTrace(System.err); + } + } + + private void debug(String format, Object... args) + { + if (DEBUG) + { + System.err.printf("[ShutdownMonitor] " + format + "%n",args); + } + } + + @Override + public void run() + { + while (true) + { + Socket socket = null; + try + { + socket = serverSocket.accept(); + + LineNumberReader lin = new LineNumberReader(new InputStreamReader(socket.getInputStream())); + String key = lin.readLine(); + if (!this.key.equals(key)) + { + System.err.println("Ignoring command with incorrect key"); + continue; + } + + OutputStream out = socket.getOutputStream(); + + String cmd = lin.readLine(); + debug("command=%s",cmd); + if ("stop".equals(cmd)) + { + // Graceful Shutdown + debug("Issuing graceful shutdown.."); + ShutdownThread.getInstance().run(); + + // Reply to client + debug("Informing client that we are stopped."); + out.write("Stopped\r\n".getBytes(StringUtil.__UTF8)); + out.flush(); + + // Shutdown Monitor + debug("Shutting down monitor"); + close(socket); + close(serverSocket); + + // Kill JVM + debug("Killing JVM"); + System.exit(0); + } + else if ("status".equals(cmd)) + { + // Reply to client + out.write("OK\r\n".getBytes(StringUtil.__UTF8)); + out.flush(); + } + } + catch (Exception e) + { + debug(e); + System.err.println(e.toString()); + } + finally + { + close(socket); + socket = null; + } + } + } + + private void close(Socket socket) + { + if (socket == null) + { + return; + } + + try + { + socket.close(); + } + catch (IOException ignore) + { + /* ignore */ + } + } + + private void close(ServerSocket server) + { + if (server == null) + { + return; + } + + try + { + server.close(); + } + catch (IOException ignore) + { + /* ignore */ + } + } +} diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java index b066a569145..4ab1764fb36 100644 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java +++ b/jetty-start/src/main/java/org/eclipse/jetty/start/Main.java @@ -53,12 +53,13 @@ import java.util.Set; /*-------------------------------------------*/ /** *

- * Main start class. This class is intended to be the main class listed in the MANIFEST.MF of the start.jar archive. It allows an application to be started with - * the command "java -jar start.jar". + * Main start class. This class is intended to be the main class listed in the MANIFEST.MF of the start.jar archive. It + * allows an application to be started with the command "java -jar start.jar". *

- * + * *

- * The behaviour of Main is controlled by the parsing of the {@link Config} "org/eclipse/start/start.config" file obtained as a resource or file. + * The behaviour of Main is controlled by the parsing of the {@link Config} "org/eclipse/start/start.config" file + * obtained as a resource or file. *

*/ public class Main @@ -91,7 +92,7 @@ public class Main Main main = new Main(); List arguments = main.expandCommandLine(args); List xmls = main.processCommandLine(arguments); - if (xmls!=null) + if (xmls != null) main.start(xmls); } catch (Throwable e) @@ -143,7 +144,7 @@ public class Main List parseStartIniFiles() { - List ini_args=new ArrayList(); + List ini_args = new ArrayList(); File start_ini = new File(_jettyHome,"start.ini"); if (start_ini.exists()) ini_args.addAll(loadStartIni(start_ini)); @@ -179,14 +180,14 @@ public class Main _showUsage = true; continue; } - + if ("--stop".equals(arg)) { int port = Integer.parseInt(Config.getProperty("STOP.PORT","-1")); String key = Config.getProperty("STOP.KEY",null); - int timeout = Integer.parseInt(Config.getProperty("STOP.WAIT", "0")); + int timeout = Integer.parseInt(Config.getProperty("STOP.WAIT","0")); stop(port,key,timeout); - return null; + return null; } if ("--version".equals(arg) || "-v".equals(arg) || "--info".equals(arg)) @@ -226,7 +227,7 @@ public class Main if (!startDir.exists() || !startDir.canWrite()) startDir = new File("."); - File startLog = new File(startDir, START_LOG_ROLLOVER_DATEFORMAT.format(new Date())); + File startLog = new File(startDir,START_LOG_ROLLOVER_DATEFORMAT.format(new Date())); if (!startLog.exists() && !startLog.createNewFile()) { @@ -246,7 +247,7 @@ public class Main PrintStream logger = new PrintStream(new FileOutputStream(startLog,false)); System.setOut(logger); System.setErr(logger); - System.out.println("Establishing "+ START_LOG_FILENAME + " on " + new Date()); + System.out.println("Establishing " + START_LOG_FILENAME + " on " + new Date()); continue; } @@ -492,11 +493,6 @@ public class Main /* ------------------------------------------------------------ */ public void start(List xmls) throws IOException, InterruptedException { - // Setup Start / Stop Monitoring - int port = Integer.parseInt(Config.getProperty("STOP.PORT","-1")); - String key = Config.getProperty("STOP.KEY",null); - Monitor monitor = new Monitor(port,key); - // Load potential Config (start.config) List configuredXmls = loadConfig(xmls); @@ -581,9 +577,8 @@ public class Main copyInThread(process.getErrorStream(),System.err); copyInThread(process.getInputStream(),System.out); copyInThread(System.in,process.getOutputStream()); - monitor.setProcess(process); process.waitFor(); - + System.exit(0); // exit JVM when child process ends. return; } @@ -688,11 +683,18 @@ public class Main cmd.addArg(x); } cmd.addRawArg("-Djetty.home=" + _jettyHome); + + // Special Stop/Shutdown properties + ensureSystemPropertySet("STOP.PORT"); + ensureSystemPropertySet("STOP.KEY"); + + // System Properties for (String p : _sysProps) { String v = System.getProperty(p); cmd.addEqualsArg("-D" + p,v); } + cmd.addArg("-cp"); cmd.addRawArg(classpath.toString()); cmd.addRawArg(_config.getMainClassname()); @@ -715,6 +717,34 @@ public class Main return cmd; } + /** + * Ensure that the System Properties are set (if defined as a System property, or start.config property, or + * start.ini property) + * + * @param key + * the key to be sure of + */ + private void ensureSystemPropertySet(String key) + { + if (_sysProps.contains(key)) + { + return; // done + } + + Properties props = Config.getProperties(); + if (props.containsKey(key)) + { + String val = props.getProperty(key,null); + if (val == null) + { + return; // no value to set + } + // setup system property + _sysProps.add(key); + System.setProperty(key,val); + } + } + private String findJavaBin() { File javaHome = new File(System.getProperty("java.home")); @@ -927,10 +957,10 @@ public class Main /** * Load Configuration. - * - * No specific configuration is real until a {@link Config#getCombinedClasspath(java.util.Collection)} is used to execute the {@link Class} specified by - * {@link Config#getMainClassname()} is executed. - * + * + * No specific configuration is real until a {@link Config#getCombinedClasspath(java.util.Collection)} is used to + * execute the {@link Class} specified by {@link Config#getMainClassname()} is executed. + * * @param xmls * the command line specified xml configuration options. * @return the list of xml configurations arriving via command line and start.config choices. @@ -1007,11 +1037,10 @@ public class Main */ public void stop(int port, String key) { - stop (port,key, 0); + stop(port,key,0); } - - public void stop (int port, String key, int timeout) + public void stop(int port, String key, int timeout) { int _port = port; String _key = key; @@ -1031,7 +1060,7 @@ public class Main Socket s = new Socket(InetAddress.getByName("127.0.0.1"),_port); if (timeout > 0) - s.setSoTimeout(timeout*1000); + s.setSoTimeout(timeout * 1000); try { OutputStream out = s.getOutputStream(); @@ -1040,7 +1069,7 @@ public class Main if (timeout > 0) { - System.err.printf("Waiting %,d seconds for jetty to stop%n", timeout); + System.err.printf("Waiting %,d seconds for jetty to stop%n",timeout); LineNumberReader lin = new LineNumberReader(new InputStreamReader(s.getInputStream())); String response; while ((response = lin.readLine()) != null) diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/Monitor.java b/jetty-start/src/main/java/org/eclipse/jetty/start/Monitor.java deleted file mode 100644 index 1f5667b09af..00000000000 --- a/jetty-start/src/main/java/org/eclipse/jetty/start/Monitor.java +++ /dev/null @@ -1,157 +0,0 @@ -// -// ======================================================================== -// Copyright (c) 1995-2012 Mort Bay Consulting Pty. Ltd. -// ------------------------------------------------------------------------ -// All rights reserved. This program and the accompanying materials -// are made available under the terms of the Eclipse Public License v1.0 -// and Apache License v2.0 which accompanies this distribution. -// -// The Eclipse Public License is available at -// http://www.eclipse.org/legal/epl-v10.html -// -// The Apache License v2.0 is available at -// http://www.opensource.org/licenses/apache2.0.php -// -// You may elect to redistribute this code under either of these licenses. -// ======================================================================== -// - -package org.eclipse.jetty.start; -import java.io.InputStreamReader; -import java.io.LineNumberReader; -import java.net.InetAddress; -import java.net.ServerSocket; -import java.net.Socket; - -/*-------------------------------------------*/ -/** Monitor thread. - * This thread listens on the port specified by the STOP.PORT system parameter - * (defaults to -1 for not listening) for request authenticated with the key given by the STOP.KEY - * system parameter (defaults to "eclipse") for admin requests. - *

- * If the stop port is set to zero, then a random port is assigned and the port number - * is printed to stdout. - *

- * Commands "stop" and * "status" are currently supported. - * - */ -public class Monitor extends Thread -{ - private Process _process; - private final int _port; - private final String _key; - - ServerSocket _socket; - - public Monitor(int port,String key) - { - try - { - if(port<0) - return; - setDaemon(true); - setName("StopMonitor"); - _socket=new ServerSocket(port,1,InetAddress.getByName("127.0.0.1")); - if (port==0) - { - port=_socket.getLocalPort(); - System.out.println(port); - } - - if (key==null) - { - key=Long.toString((long)(Long.MAX_VALUE*Math.random()+this.hashCode()+System.currentTimeMillis()),36); - System.out.println("STOP.KEY="+key); - } - } - catch(Exception e) - { - Config.debug(e); - System.err.println("Error binding monitor port "+port+": "+e.toString()); - } - finally - { - _port=port; - _key=key; - } - - if (_socket!=null) - this.start(); - else - System.err.println("WARN: Not listening on monitor port: "+_port); - } - - public Process getProcess() - { - return _process; - } - - public void setProcess(Process process) - { - _process = process; - } - - @Override - public void run() - { - while (true) - { - Socket socket=null; - try{ - socket=_socket.accept(); - - LineNumberReader lin= - new LineNumberReader(new InputStreamReader(socket.getInputStream())); - String key=lin.readLine(); - if (!_key.equals(key)) - { - System.err.println("Ignoring command with incorrect key"); - continue; - } - - String cmd=lin.readLine(); - Config.debug("command=" + cmd); - if ("stop".equals(cmd)) - { - Config.debug("Child Process: " + _process); - if (_process!=null) - { - //if we have a child process, wait for it to finish before we stop - try - { - _process.destroy(); - _process.waitFor(); - } - catch (InterruptedException e) - { - System.err.println("Interrupted waiting for child to terminate"); - } - } - socket.getOutputStream().write("Stopped\r\n".getBytes()); - try {socket.close();}catch(Exception e){e.printStackTrace();} - try {_socket.close();}catch(Exception e){e.printStackTrace();} - System.exit(0); - } - else if ("status".equals(cmd)) - { - socket.getOutputStream().write("OK\r\n".getBytes()); - socket.getOutputStream().flush(); - } - } - catch(Exception e) - { - Config.debug(e); - System.err.println(e.toString()); - } - finally - { - if (socket!=null) - { - try{socket.close();}catch(Exception e){} - } - socket=null; - } - } - } - -} From 128cf0a6b4c9beb3953dfc71bc8087b0f599da67 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 18 Dec 2012 18:19:17 -0700 Subject: [PATCH 3/4] 396886 - MultiPartFilter strips bad escaping on filename="..." --- .../jetty/servlets/MultiPartFilter.java | 13 ++-- .../jetty/servlets/MultipartFilterTest.java | 60 ++++++++++++++++--- .../jetty/util/QuotedStringTokenizer.java | 15 +++++ .../jetty/util/QuotedStringTokenizerTest.java | 21 ++++++- .../eclipse/jetty/testing/ServletTester.java | 6 ++ 5 files changed, 101 insertions(+), 14 deletions(-) diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java index f1855c3406f..36392aa0deb 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java @@ -18,7 +18,6 @@ package org.eclipse.jetty.servlets; -import java.io.BufferedInputStream; import java.io.BufferedOutputStream; import java.io.BufferedReader; import java.io.ByteArrayOutputStream; @@ -55,7 +54,8 @@ import org.eclipse.jetty.util.MultiMap; import org.eclipse.jetty.util.QuotedStringTokenizer; import org.eclipse.jetty.util.ReadLineInputStream; import org.eclipse.jetty.util.StringUtil; -import org.eclipse.jetty.util.TypeUtil; +import org.eclipse.jetty.util.log.Log; +import org.eclipse.jetty.util.log.Logger; /* ------------------------------------------------------------ */ /** @@ -79,6 +79,7 @@ import org.eclipse.jetty.util.TypeUtil; */ public class MultiPartFilter implements Filter { + private static final Logger LOG = Log.getLogger(MultiPartFilter.class); public final static String CONTENT_TYPE_SUFFIX=".org.eclipse.jetty.servlet.contentType"; private final static String FILES ="org.eclipse.jetty.servlet.MultiPartFilter.files"; private File tempdir; @@ -197,7 +198,8 @@ public class MultiPartFilter implements Filter throw new IOException("Missing content-disposition"); } - QuotedStringTokenizer tok=new QuotedStringTokenizer(content_disposition,";"); + LOG.debug("Content-Disposition: {}", content_disposition); + QuotedStringTokenizer tok=new QuotedStringTokenizer(content_disposition,";",false,true); String name=null; String filename=null; while(tok.hasMoreTokens()) @@ -233,6 +235,7 @@ public class MultiPartFilter implements Filter { if (filename!=null && filename.length()>0) { + LOG.debug("filename = \"{}\"", filename); file = File.createTempFile("MultiPart", "", tempdir); out = new FileOutputStream(file); if(_fileOutputBuffer>0) @@ -409,7 +412,9 @@ public class MultiPartFilter implements Filter /* ------------------------------------------------------------ */ private String value(String nameEqualsValue) { - return nameEqualsValue.substring(nameEqualsValue.indexOf('=')+1).trim(); + int idx = nameEqualsValue.indexOf('='); + String value = nameEqualsValue.substring(idx+1).trim(); + return QuotedStringTokenizer.unquoteOnly(value); } /* ------------------------------------------------------------------------------- */ diff --git a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java index e61f793c5f3..e7f771d0cef 100644 --- a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java +++ b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java @@ -18,13 +18,13 @@ package org.eclipse.jetty.servlets; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; -import static org.junit.Assert.assertNotNull; +import static org.hamcrest.Matchers.*; +import static org.junit.Assert.*; import java.io.File; import java.io.FileInputStream; import java.io.IOException; +import java.io.PrintWriter; import javax.servlet.ServletException; import javax.servlet.http.HttpServlet; @@ -36,7 +36,6 @@ import org.eclipse.jetty.servlet.FilterMapping; import org.eclipse.jetty.testing.HttpTester; import org.eclipse.jetty.testing.ServletTester; import org.eclipse.jetty.util.IO; -import org.eclipse.jetty.util.QuotedStringTokenizer; import org.junit.After; import org.junit.Before; import org.junit.Test; @@ -197,6 +196,42 @@ public class MultipartFilterTest assertEquals(HttpServletResponse.SC_OK,response.getStatus()); assertTrue(response.getContent().indexOf("brown cow")>=0); } + + @Test + public void testBadlyEncodedFilename() throws Exception + { + // generated and parsed test + HttpTester request = new HttpTester(); + HttpTester response = new HttpTester(); + + // test GET + request.setMethod("POST"); + request.setVersion("HTTP/1.0"); + request.setHeader("Host","tester"); + request.setURI("/context/dump"); + + String boundary="XyXyXy"; + request.setHeader("Content-Type","multipart/form-data; boundary="+boundary); + + + String content = "--" + boundary + "\r\n"+ + "Content-Disposition: form-data; name=\"fileup\"; filename=\"Taken on Aug 22 \\ 2012.jpg\"\r\n"+ + "Content-Type: application/octet-stream\r\n\r\n"+ + "How now brown cow."+ + "\r\n--" + boundary + "--\r\n\r\n"; + + request.setContent(content); + + response.parse(tester.getResponses(request.generate())); + + System.out.printf("Content: [%s]%n", response.getContent()); + + assertThat(response.getMethod(), nullValue()); + assertThat(response.getStatus(), is(HttpServletResponse.SC_OK)); + + assertThat(response.getContent(), containsString("Filename [Taken on Aug 22 \\ 2012.jpg]")); + assertThat(response.getContent(), containsString("How now brown cow.")); + } /* * Test multipart with parts encoded in base64 (RFC1521 section 5) @@ -553,7 +588,9 @@ public class MultipartFilterTest @Override protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { - assertEquals("How now brown cow.", req.getParameterMap().get("strupContent-Type: application/octet-stream")); + String content = (String)req.getParameterMap().get("\"strup\"Content-Type: application/octet-stream"); + + assertThat(content, containsString("How now brown cow.")); super.doPost(req, resp); } @@ -613,8 +650,17 @@ public class MultipartFilterTest @Override protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException { - resp.getWriter().println((IO.toString(new FileInputStream((File)req.getAttribute("fileup"))))); + FileInputStream in = null; + try { + File file = (File)req.getAttribute("fileup"); + in = new FileInputStream(file); + + PrintWriter out = resp.getWriter(); + out.printf("Filename [%s]\r\n", req.getParameter("fileup")); + out.println(IO.toString(in)); + } finally { + IO.close(in); + } } - } } diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java b/jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java index 8dc0a816f8f..1dc416ec88a 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java @@ -435,6 +435,10 @@ public class QuotedStringTokenizer if (escape) { escape=false; + if (!isValidEscaping(c)) + { + b.append('\\'); + } b.append(c); } else if (c=='\\') @@ -512,6 +516,10 @@ public class QuotedStringTokenizer ); break; default: + if (!isValidEscaping(c)) + { + b.append('\\'); + } b.append(c); } } @@ -527,6 +535,13 @@ public class QuotedStringTokenizer return b.toString(); } + + private static boolean isValidEscaping(char c) + { + return ((c == 'n') || (c == 'r') || (c == 't') || + (c == 'f') || (c == 'b') || (c == '\\') || + (c == '/') || (c == '"') || (c == 'u')); + } /* ------------------------------------------------------------ */ /** diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/QuotedStringTokenizerTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/QuotedStringTokenizerTest.java index 3407574bea9..989dc55cb33 100644 --- a/jetty-util/src/test/java/org/eclipse/jetty/util/QuotedStringTokenizerTest.java +++ b/jetty-util/src/test/java/org/eclipse/jetty/util/QuotedStringTokenizerTest.java @@ -18,9 +18,7 @@ package org.eclipse.jetty.util; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; import org.junit.Test; @@ -190,4 +188,21 @@ public class QuotedStringTokenizerTest assertEquals("ba\\uXXXXaaa", QuotedStringTokenizer.unquoteOnly("\"ba\\\\uXXXXaaa\"")); } + /** + * When encountering a Content-Disposition line during a multi-part mime file + * upload, the filename="..." field can contain '\' characters that do not + * belong to a proper escaping sequence, this tests QuotedStringTokenizer to + * ensure that it preserves those slashes for where they cannot be escaped. + */ + @Test + public void testNextTokenOnContentDisposition() + { + String content_disposition = "form-data; name=\"fileup\"; filename=\"Taken on Aug 22 \\ 2012.jpg\""; + + QuotedStringTokenizer tok=new QuotedStringTokenizer(content_disposition,";",false,true); + + assertEquals("form-data", tok.nextToken().trim()); + assertEquals("name=\"fileup\"", tok.nextToken().trim()); + assertEquals("filename=\"Taken on Aug 22 \\ 2012.jpg\"", tok.nextToken().trim()); + } } diff --git a/test-jetty-servlet/src/main/java/org/eclipse/jetty/testing/ServletTester.java b/test-jetty-servlet/src/main/java/org/eclipse/jetty/testing/ServletTester.java index bd07b9cdb88..7df550a6850 100644 --- a/test-jetty-servlet/src/main/java/org/eclipse/jetty/testing/ServletTester.java +++ b/test-jetty-servlet/src/main/java/org/eclipse/jetty/testing/ServletTester.java @@ -97,6 +97,12 @@ public class ServletTester _server.start(); } + /* ------------------------------------------------------------ */ + public void join() throws Exception + { + _server.join(); + } + /* ------------------------------------------------------------ */ public void stop() throws Exception { From 720263151596d37014013c1a0033a8dc0fab9d5e Mon Sep 17 00:00:00 2001 From: Jan Bartel Date: Thu, 20 Dec 2012 13:48:45 +1100 Subject: [PATCH 4/4] 396886 MultiPartFilter strips bad escaping on filename="..." --- .../jetty/servlets/MultiPartFilter.java | 30 +++++++- .../jetty/servlets/MultipartFilterTest.java | 75 ++++++++++++++++++- .../jetty/util/QuotedStringTokenizer.java | 32 ++++++-- 3 files changed, 130 insertions(+), 7 deletions(-) diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java index 36392aa0deb..b2ac85b3051 100644 --- a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java +++ b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/MultiPartFilter.java @@ -211,7 +211,7 @@ public class MultiPartFilter implements Filter else if(tl.startsWith("name=")) name=value(t); else if(tl.startsWith("filename=")) - filename=value(t); + filename=filenameValue(t); } // Check disposition @@ -416,6 +416,34 @@ public class MultiPartFilter implements Filter String value = nameEqualsValue.substring(idx+1).trim(); return QuotedStringTokenizer.unquoteOnly(value); } + + + /* ------------------------------------------------------------ */ + private String filenameValue(String nameEqualsValue) + { + int idx = nameEqualsValue.indexOf('='); + String value = nameEqualsValue.substring(idx+1).trim(); + + if (value.matches(".??[a-z,A-Z]\\:\\\\[^\\\\].*")) + { + //incorrectly escaped IE filenames that have the whole path + //we just strip any leading & trailing quotes and leave it as is + char first=value.charAt(0); + if (first=='"' || first=='\'') + value=value.substring(1); + char last=value.charAt(value.length()-1); + if (last=='"' || last=='\'') + value = value.substring(0,value.length()-1); + + return value; + } + else + //unquote the string, but allow any backslashes that don't + //form a valid escape sequence to remain as many browsers + //even on *nix systems will not escape a filename containing + //backslashes + return QuotedStringTokenizer.unquoteOnly(value, true); + } /* ------------------------------------------------------------------------------- */ /** diff --git a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java index e7f771d0cef..19e144c1fc4 100644 --- a/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java +++ b/jetty-servlets/src/test/java/org/eclipse/jetty/servlets/MultipartFilterTest.java @@ -224,7 +224,7 @@ public class MultipartFilterTest response.parse(tester.getResponses(request.generate())); - System.out.printf("Content: [%s]%n", response.getContent()); + //System.out.printf("Content: [%s]%n", response.getContent()); assertThat(response.getMethod(), nullValue()); assertThat(response.getStatus(), is(HttpServletResponse.SC_OK)); @@ -232,7 +232,80 @@ public class MultipartFilterTest assertThat(response.getContent(), containsString("Filename [Taken on Aug 22 \\ 2012.jpg]")); assertThat(response.getContent(), containsString("How now brown cow.")); } + + @Test + public void testBadlyEncodedMSFilename() throws Exception + { + // generated and parsed test + HttpTester request = new HttpTester(); + HttpTester response = new HttpTester(); + // test GET + request.setMethod("POST"); + request.setVersion("HTTP/1.0"); + request.setHeader("Host","tester"); + request.setURI("/context/dump"); + + String boundary="XyXyXy"; + request.setHeader("Content-Type","multipart/form-data; boundary="+boundary); + + + String content = "--" + boundary + "\r\n"+ + "Content-Disposition: form-data; name=\"fileup\"; filename=\"c:\\this\\really\\is\\some\\path\\to\\a\\file.txt\"\r\n"+ + "Content-Type: application/octet-stream\r\n\r\n"+ + "How now brown cow."+ + "\r\n--" + boundary + "--\r\n\r\n"; + + request.setContent(content); + + response.parse(tester.getResponses(request.generate())); + + //System.out.printf("Content: [%s]%n", response.getContent()); + + assertThat(response.getMethod(), nullValue()); + assertThat(response.getStatus(), is(HttpServletResponse.SC_OK)); + + assertThat(response.getContent(), containsString("Filename [c:\\this\\really\\is\\some\\path\\to\\a\\file.txt]")); + assertThat(response.getContent(), containsString("How now brown cow.")); + } + + @Test + public void testCorrectlyEncodedMSFilename() throws Exception + { + // generated and parsed test + HttpTester request = new HttpTester(); + HttpTester response = new HttpTester(); + + // test GET + request.setMethod("POST"); + request.setVersion("HTTP/1.0"); + request.setHeader("Host","tester"); + request.setURI("/context/dump"); + + String boundary="XyXyXy"; + request.setHeader("Content-Type","multipart/form-data; boundary="+boundary); + + + String content = "--" + boundary + "\r\n"+ + "Content-Disposition: form-data; name=\"fileup\"; filename=\"c:\\\\this\\\\really\\\\is\\\\some\\\\path\\\\to\\\\a\\\\file.txt\"\r\n"+ + "Content-Type: application/octet-stream\r\n\r\n"+ + "How now brown cow."+ + "\r\n--" + boundary + "--\r\n\r\n"; + + request.setContent(content); + + response.parse(tester.getResponses(request.generate())); + + //System.out.printf("Content: [%s]%n", response.getContent()); + + assertThat(response.getMethod(), nullValue()); + assertThat(response.getStatus(), is(HttpServletResponse.SC_OK)); + + assertThat(response.getContent(), containsString("Filename [c:\\this\\really\\is\\some\\path\\to\\a\\file.txt]")); + assertThat(response.getContent(), containsString("How now brown cow.")); + } + + /* * Test multipart with parts encoded in base64 (RFC1521 section 5) */ diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java b/jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java index 1dc416ec88a..54ee74fe339 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/QuotedStringTokenizer.java @@ -409,12 +409,21 @@ public class QuotedStringTokenizer } } + + /* ------------------------------------------------------------ */ + public static String unquoteOnly(String s) + { + return unquoteOnly(s, false); + } + + /* ------------------------------------------------------------ */ /** Unquote a string, NOT converting unicode sequences * @param s The string to unquote. + * @param lenient if true, will leave in backslashes that aren't valid escapes * @return quoted string */ - public static String unquoteOnly(String s) + public static String unquoteOnly(String s, boolean lenient) { if (s==null) return null; @@ -435,7 +444,7 @@ public class QuotedStringTokenizer if (escape) { escape=false; - if (!isValidEscaping(c)) + if (lenient && !isValidEscaping(c)) { b.append('\\'); } @@ -453,13 +462,19 @@ public class QuotedStringTokenizer return b.toString(); } - + + /* ------------------------------------------------------------ */ + public static String unquote(String s) + { + return unquote(s,false); + } + /* ------------------------------------------------------------ */ /** Unquote a string. * @param s The string to unquote. * @return quoted string */ - public static String unquote(String s) + public static String unquote(String s, boolean lenient) { if (s==null) return null; @@ -516,7 +531,7 @@ public class QuotedStringTokenizer ); break; default: - if (!isValidEscaping(c)) + if (lenient && !isValidEscaping(c)) { b.append('\\'); } @@ -536,6 +551,13 @@ public class QuotedStringTokenizer return b.toString(); } + + /* ------------------------------------------------------------ */ + /** Check that char c (which is preceded by a backslash) is a valid + * escape sequence. + * @param c + * @return + */ private static boolean isValidEscaping(char c) { return ((c == 'n') || (c == 'r') || (c == 't') ||