From e0a9497ee9511de19288d001e728198f7b518329 Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Sun, 25 Jan 2026 17:45:59 -0600 Subject: [PATCH 1/2] Safer ExternalPermutationWorkerFactory connections Cookies are now sent/received as 32 ascii chars before any ObjectOutputStream data is sent, and the server validates before returning the socket. An invalid socket is closed and ignored, and the next valid socket will be used instead. Also improved cookie randomness source. Fixes #10256 --- .../google/gwt/dev/CompilePermsServer.java | 16 ++-- .../dev/ExternalPermutationWorkerFactory.java | 86 +++++++++---------- 2 files changed, 52 insertions(+), 50 deletions(-) diff --git a/dev/core/src/com/google/gwt/dev/CompilePermsServer.java b/dev/core/src/com/google/gwt/dev/CompilePermsServer.java index fe7889fdb0e..42cdd10fba1 100644 --- a/dev/core/src/com/google/gwt/dev/CompilePermsServer.java +++ b/dev/core/src/com/google/gwt/dev/CompilePermsServer.java @@ -33,8 +33,10 @@ import java.io.IOException; import java.io.ObjectInputStream; import java.io.ObjectOutputStream; +import java.io.OutputStream; import java.net.Socket; import java.net.UnknownHostException; +import java.nio.charset.StandardCharsets; /** * An out-of-process implementation of CompilePerms that will connect back to an @@ -246,16 +248,16 @@ public static void main(String[] args) { } public static boolean run(CompileServerOptions options, TreeLogger logger) { - try { - Socket s = new Socket(options.getCompileHost(), options.getCompilePort()); + try (Socket s = new Socket(options.getCompileHost(), options.getCompilePort())) { logger.log(TreeLogger.DEBUG, "Socket opened"); - ObjectOutputStream out = new ObjectOutputStream(s.getOutputStream()); - ObjectInputStream in = new StringInterningObjectInputStream(s.getInputStream()); - // Write my cookie - out.writeUTF(options.getCookie()); - out.flush(); + OutputStream rawOut = s.getOutputStream(); + rawOut.write(options.getCookie().getBytes(StandardCharsets.US_ASCII)); + rawOut.flush(); + + ObjectOutputStream out = new ObjectOutputStream(rawOut); + ObjectInputStream in = new StringInterningObjectInputStream(s.getInputStream()); // Read the File that contains the serialized UnifiedAst File astFile = (File) in.readObject(); diff --git a/dev/core/src/com/google/gwt/dev/ExternalPermutationWorkerFactory.java b/dev/core/src/com/google/gwt/dev/ExternalPermutationWorkerFactory.java index fa7473750eb..dca454aa614 100644 --- a/dev/core/src/com/google/gwt/dev/ExternalPermutationWorkerFactory.java +++ b/dev/core/src/com/google/gwt/dev/ExternalPermutationWorkerFactory.java @@ -37,6 +37,8 @@ import java.net.ServerSocket; import java.net.Socket; import java.net.SocketTimeoutException; +import java.nio.charset.StandardCharsets; +import java.security.SecureRandom; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -44,7 +46,6 @@ import java.util.HashSet; import java.util.Iterator; import java.util.List; -import java.util.Random; import java.util.Set; /** @@ -61,47 +62,53 @@ public class ExternalPermutationWorkerFactory extends PermutationWorkerFactory { * before closing the socket. */ private static class CountedServerSocket { - private int accepts; - private ServerSocket sock; + private final ServerSocket sock; + private final Set validCookies; + private final TreeLogger logger; - public CountedServerSocket(ServerSocket sock, int maxAccepts) { + public CountedServerSocket(TreeLogger logger, ServerSocket sock, Set validCookies) { assert sock != null; - assert maxAccepts >= 1; + assert !validCookies.isEmpty(); - this.accepts = maxAccepts; + this.logger = logger; this.sock = sock; + this.validCookies = validCookies; } + /** + * Returns an authenticated Socket connection. + * @return the next connecting socket to present a valid unused cookie + * @throws IOException if there is an error with the socket + */ public synchronized Socket accept() throws IOException { - assert accepts >= 0; - - if (accepts == 0) { - throw new IllegalStateException("Too many calls to accept()"); - } - - try { - return sock.accept(); - } finally { - if (--accepts == 0) { - sock.close(); - sock = null; + while (!validCookies.isEmpty()) { + Socket s = sock.accept(); + byte[] bytes = s.getInputStream().readNBytes(32); + String cookie = new String(bytes, StandardCharsets.US_ASCII); + if (validCookies.remove(cookie)) { + if (validCookies.isEmpty()) { + // We've removed the final cookie, no further connections are expected + sock.close(); + } + return s; } + s.close(); + logger.log(TreeLogger.WARN, "Rejected connection from " + + s.getRemoteSocketAddress() + " with invalid cookie: " + cookie); } + throw new IllegalStateException("No more cookies, too many calls to accept()"); } } private static class ExternalPermutationWorker implements PermutationWorker { private final File astFile; - private final Set cookies; private ObjectInputStream in; private ObjectOutputStream out; private final CountedServerSocket serverSocket; private Socket workerSocket; - public ExternalPermutationWorker(CountedServerSocket sock, File astFile, - Set cookies) { + public ExternalPermutationWorker(CountedServerSocket sock, File astFile) { this.astFile = astFile; - this.cookies = cookies; this.serverSocket = sock; } @@ -121,14 +128,6 @@ public void compile(TreeLogger logger, CompilerContext compilerContext, Permutat in = new StringInterningObjectInputStream(workerSocket.getInputStream()); out = new ObjectOutputStream(workerSocket.getOutputStream()); - - // Verify we're talking to the right worker - String c = in.readUTF(); - if (!cookies.contains(c)) { - throw new TransientWorkerException("Received unknown cookie " + c, - null); - } - out.writeObject(astFile); // Get the remote worker's estimate of memory use @@ -220,13 +219,12 @@ public void shutdown() { /** * Random number generator used for keys to worker threads. */ - private static Random random = new Random(); + private static final SecureRandom random = new SecureRandom(); /** - * Launches an external worker and returns the cookie that worker should - * return via the network connection. + * Launches an external worker, with a port to connect to and a cookie to authenticate with. */ - private static String launchExternalWorker(TreeLogger logger, int port) + private static void launchExternalWorker(TreeLogger logger, String cookie, int port) throws UnableToCompleteException { String javaCommand = System.getProperty(JAVA_COMMAND_PROPERTY, @@ -258,10 +256,6 @@ private static String launchExternalWorker(TreeLogger logger, int port) } } - byte[] cookieBytes = new byte[16]; - random.nextBytes(cookieBytes); - String cookie = StringUtils.toHexString(cookieBytes); - // Cook up the classpath, main class, and extra args args.addAll(Arrays.asList( CompilePermsServer.class.getName(), "-host", "localhost", "-port", @@ -342,7 +336,6 @@ public void run() { } })); - return cookie; } catch (IOException e) { logger.log(TreeLogger.ERROR, "Unable to start external process", e); throw new UnableToCompleteException(); @@ -375,15 +368,22 @@ public Collection getWorkers(TreeLogger logger, Set cookies = Collections.synchronizedSet(new HashSet( numWorkers)); - CountedServerSocket countedSock = new CountedServerSocket(sock, numWorkers); + for (int i = 0; i < numWorkers; i++) { + byte[] cookieBytes = new byte[16]; + random.nextBytes(cookieBytes); + cookies.add(StringUtils.toHexString(cookieBytes)); + } + CountedServerSocket countedSock = new CountedServerSocket(logger, sock, cookies); List toReturn = new ArrayList( numWorkers); + // Copy the cookies to a list so we can guarantee we never have a race with removals + List cookieList = new ArrayList<>(cookies); + // TODO(spoon): clean up already-launched processes if we get an exception? for (int i = 0; i < numWorkers; i++) { - String cookie = launchExternalWorker(logger, sock.getLocalPort()); - cookies.add(cookie); - toReturn.add(new ExternalPermutationWorker(countedSock, astFile, cookies)); + launchExternalWorker(logger, cookieList.get(i), sock.getLocalPort()); + toReturn.add(new ExternalPermutationWorker(countedSock, astFile)); } return toReturn; From 3228a83c63221e3e87992b2d203daefe3a68e14b Mon Sep 17 00:00:00 2001 From: Colin Alworth Date: Fri, 13 Mar 2026 14:24:34 -0500 Subject: [PATCH 2/2] style fixes --- .../google/gwt/dev/ExternalPermutationWorkerFactory.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dev/core/src/com/google/gwt/dev/ExternalPermutationWorkerFactory.java b/dev/core/src/com/google/gwt/dev/ExternalPermutationWorkerFactory.java index dca454aa614..35ae2d8f352 100644 --- a/dev/core/src/com/google/gwt/dev/ExternalPermutationWorkerFactory.java +++ b/dev/core/src/com/google/gwt/dev/ExternalPermutationWorkerFactory.java @@ -107,14 +107,14 @@ private static class ExternalPermutationWorker implements PermutationWorker { private final CountedServerSocket serverSocket; private Socket workerSocket; - public ExternalPermutationWorker(CountedServerSocket sock, File astFile) { + ExternalPermutationWorker(CountedServerSocket sock, File astFile) { this.astFile = astFile; this.serverSocket = sock; } @Override - public void compile(TreeLogger logger, CompilerContext compilerContext, Permutation permutation, - PersistenceBackedObject resultFile) + public void compile(TreeLogger logger, CompilerContext compilerContext, + Permutation permutation, PersistenceBackedObject resultFile) throws TransientWorkerException, UnableToCompleteException { // If we've just started, we need to get a connection from a subprocess