Fix resource leaks and NPEs.

Detected using Facebook's static analysis tools via a build using
Buck.

java/client/src/com/thoughtworks/selenium/webdriven/commands/AttachFile.java

@@ -32,8 +32,11 @@
 import java.io.IOException;
 import java.net.MalformedURLException;
 import java.net.URL;
+import java.util.logging.Level;
+import java.util.logging.Logger;
 
 public class AttachFile extends SeleneseCommand<Void> {
+  private final static Logger LOGGER = Logger.getLogger(AttachFile.class.getName());
   private final ElementFinder finder;
 
   public AttachFile(ElementFinder finder) {
@@ -63,6 +66,15 @@ private File downloadFile(String name) {
       Resources.copy(url, fos);
     } catch (IOException e) {
       throw new SeleniumException("Can't access file to upload: " + url, e);
+    } finally {
+      try {
+        if (fos != null) {
+          fos.close();
+        }
+      } catch (IOException e) {
+        // Nothing sane to do. Log and continue.
+        LOGGER.log(Level.WARNING, "Unable to close stream used for reading file: " + name, e);
+      }
     }
 
     return outputTo;

java/client/test/org/openqa/selenium/environment/webserver/UploadServlet.java

@@ -46,10 +46,17 @@ protected void doPost(HttpServletRequest request,
 
     int length = (int) file.length();
     byte[] buffer = new byte[length];
-    InputStream in = new FileInputStream(file);
-    in.read(buffer, 0, length);
-    String content = new String(buffer, "UTF-8");
-    in.close();
+    InputStream in = null;
+    String content;
+    try {
+      in = new FileInputStream(file);
+      in.read(buffer, 0, length);
+      content = new String(buffer, "UTF-8");
+    } finally {
+      if (in != null) {
+        in.close();
+      }
+    }
 
     // Slow down the upload so we can verify WebDriver waits.
     try {

java/server/src/cybervillains/ca/Generator.java

@@ -38,12 +38,12 @@
  * $> rm -r new_certs
  * $> COMMIT TO SELENIUM REPO
  * </pre>
- * 
+ *
  * *************************************************************************************** Copyright
  * (c) 2012, NeuStar, Inc. All Rights Reserved.
- * 
+ *
  * In a special exception, Selenium/OpenQA is allowed to use this code under the Apache License 2.0.
- * 
+ *
  * @author Mark Watson <[email protected]>, Ivan De Marino <[email protected]>
  */
 public class Generator {
@@ -62,6 +62,9 @@ public static void main(String[] args) {
         X509Certificate caCrlCert = null;
         try {
             caCrlCert = mgr.getSigningCert();
+            if (caCrlCert == null) {
+                throw new IllegalStateException("Unable to obtain signing certificate");
+            }
             PrivateKey caCrlPrivateKey = mgr.getSigningPrivateKey();
 
             crlGen.setIssuerDN(mgr.getSigningCert().getSubjectX500Principal());

java/server/src/cybervillains/ca/KeyStoreManager.java

@@ -39,28 +39,29 @@
 import java.security.cert.X509Certificate;
 import java.security.spec.DSAParameterSpec;
 import java.util.HashMap;
+import java.util.Map;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 
 import javax.crypto.spec.DHParameterSpec;
 
 /**
  * This is the main entry point into the Cybervillains CA.
- * 
+ *
  * This class handles generation, storage and the persistent mapping of input to duplicated
  * certificates and mapped public keys.
- * 
+ *
  * Default setting is to immediately persist changes to the store by writing out the keystore and
  * mapping file every time a new certificate is added. This behavior can be disabled if desired, to
  * enhance performance or allow temporary testing without modifying the certificate store.
- * 
- *************************************************************************************** 
+ *
+ ***************************************************************************************
  * Copyright (c) 2007, Information Security Partners, LLC All rights reserved.
- * 
+ *
  * In a special exception, Selenium/OpenQA is allowed to use this code under the Apache License 2.0.
- * 
+ *
  * @author Brad Hill
- * 
+ *
  */
 public class KeyStoreManager {
 
@@ -384,7 +385,7 @@ protected void createKeystore() {
 
   /**
    * Stores a new certificate and its associated private key in the keystore.
-   * 
+   *
    * @throws KeyStoreException
    * @throws CertificateException
    * @throws NoSuchAlgorithmException
@@ -409,32 +410,39 @@ public synchronized void addCertAndPrivateKey(String hostname, final X509Certifi
 
   /**
    * Writes the keystore and certificate/keypair mappings to disk.
-   * 
+   *
    * @throws KeyStoreException
    * @throws NoSuchAlgorithmException
    * @throws CertificateException
    */
   public synchronized void persist() throws KeyStoreException, NoSuchAlgorithmException,
       CertificateException {
-    try
-    {
-      FileOutputStream kso = new FileOutputStream(new File(root, _caPrivateKeystore));
+    FileOutputStream kso = null;
+    try {
+      kso = new FileOutputStream(new File(root, _caPrivateKeystore));
       _ks.store(kso, _keystorepass);
       kso.flush();
       kso.close();
       persistCertMap();
       persistSubjectMap();
       persistKeyPairMap();
       persistPublicKeyMap();
-    } catch (IOException ioe)
-    {
+    } catch (IOException ioe) {
       ioe.printStackTrace();
+    } finally {
+      if (kso != null) {
+        try {
+          kso.close();
+        } catch (IOException e) {
+          log.log(Level.WARNING, "Unable to close " + _caPrivateKeystore, e);
+        }
+      }
     }
   }
 
   /**
    * Returns the aliased certificate. Certificates are aliased by their SHA1 digest.
-   * 
+   *
    * @see ThumbprintUtil
    * @throws KeyStoreException
    */
@@ -445,7 +453,7 @@ public synchronized X509Certificate getCertificateByAlias(final String alias)
 
   /**
    * Returns the aliased certificate. Certificates are aliased by their hostname.
-   * 
+   *
    * @see ThumbprintUtil
    * @throws KeyStoreException
    * @throws UnrecoverableKeyException
@@ -472,7 +480,7 @@ public synchronized X509Certificate getCertificateByHostname(final String hostna
 
   /**
    * Gets the authority root signing cert.
-   * 
+   *
    * @throws KeyStoreException
    */
   public synchronized X509Certificate getSigningCert() throws KeyStoreException {
@@ -481,7 +489,7 @@ public synchronized X509Certificate getSigningCert() throws KeyStoreException {
 
   /**
    * Gets the authority private signing key.
-   * 
+   *
    * @throws KeyStoreException
    * @throws NoSuchAlgorithmException
    * @throws UnrecoverableKeyException
@@ -495,7 +503,7 @@ public synchronized PrivateKey getSigningPrivateKey() throws KeyStoreException,
    * This method returns the mapped certificate for a hostname, or generates a "standard" SSL server
    * certificate issued by the CA to the supplied subject if no mapping has been created. This is
    * not a true duplication, just a shortcut method that is adequate for web browsers.
-   * 
+   *
    * @throws CertificateParsingException
    * @throws InvalidKeyException
    * @throws CertificateExpiredException
@@ -544,39 +552,13 @@ private String getSubjectForHostname(String hostname) {
   }
 
   private synchronized void persistCertMap() {
-    try {
-      ObjectOutput out =
-          new ObjectOutputStream(new FileOutputStream(new File(root, CERTMAP_SER_FILE)));
-      out.writeObject(_certMap);
-      out.flush();
-      out.close();
-    } catch (FileNotFoundException e) {
-      // writing, this shouldn't happen...
-      e.printStackTrace();
-    } catch (IOException e) {
-      // big problem!
-      e.printStackTrace();
-      throw new Error(e);
-    }
+    persistMap(new File(root, CERTMAP_SER_FILE), _certMap);
   }
 
 
 
   private synchronized void persistSubjectMap() {
-    try {
-      ObjectOutput out =
-          new ObjectOutputStream(new FileOutputStream(new File(root, SUBJMAP_SER_FILE)));
-      out.writeObject(_subjectMap);
-      out.flush();
-      out.close();
-    } catch (FileNotFoundException e) {
-      // writing, this shouldn't happen...
-      e.printStackTrace();
-    } catch (IOException e) {
-      // big problem!
-      e.printStackTrace();
-      throw new Error(e);
-    }
+    persistMap(new File(root, SUBJMAP_SER_FILE), _subjectMap);
   }
 
   /**
@@ -591,36 +573,35 @@ public KeyPair getRSAKeyPair()
   }
 
   private synchronized void persistPublicKeyMap() {
-    try {
-      ObjectOutput out =
-          new ObjectOutputStream(new FileOutputStream(new File(root, PUB_KEYMAP_SER_FILE)));
-      out.writeObject(_mappedPublicKeys);
-      out.flush();
-      out.close();
-    } catch (FileNotFoundException e) {
-      // writing, won't happen
-      e.printStackTrace();
-    } catch (IOException e) {
-      // very bad
-      e.printStackTrace();
-      throw new Error(e);
-    }
+    persistMap(new File(root, PUB_KEYMAP_SER_FILE), _mappedPublicKeys);
   }
 
   private synchronized void persistKeyPairMap() {
+    persistMap(new File(root, KEYMAP_SER_FILE), _rememberedPrivateKeys);
+  }
+
+  private void persistMap(File outputFile, Map<?, ?> toPersist) {
+    ObjectOutput out = null;
     try {
-      ObjectOutput out =
-          new ObjectOutputStream(new FileOutputStream(new File(root, KEYMAP_SER_FILE)));
-      out.writeObject(_rememberedPrivateKeys);
+      out = new ObjectOutputStream(new FileOutputStream(outputFile));
+      out.writeObject(toPersist);
       out.flush();
-      out.close();
     } catch (FileNotFoundException e) {
       // writing, won't happen.
       e.printStackTrace();
     } catch (IOException e) {
       // very bad
       e.printStackTrace();
       throw new Error(e);
+    } finally {
+      if (out != null) {
+        try {
+          out.close();
+        } catch (IOException e) {
+          // Nothing sane to do.
+          log.log(Level.WARNING, "Unable to close " + outputFile.getName(), e);
+        }
+      }
     }
   }
 

java/server/src/org/openqa/grid/web/servlet/handler/WebDriverRequest.java

@@ -46,7 +46,7 @@ public RequestType extractRequestType() {
       return RequestType.START_SESSION;
     } else if (getMethod().equalsIgnoreCase("DELETE")) {
       ExternalSessionKey externalKey = ExternalSessionKey.fromWebDriverRequest(getPathInfo());
-      if (getPathInfo().endsWith("/session/" + externalKey.getKey())) {
+      if (externalKey != null && getPathInfo().endsWith("/session/" + externalKey.getKey())) {
         return RequestType.STOP_SESSION;
       }
     }

java/server/src/org/openqa/selenium/remote/server/handler/WebDriverHandler.java

@@ -17,6 +17,8 @@
 
 package org.openqa.selenium.remote.server.handler;
 
+import com.google.common.base.Preconditions;
+
 import org.openqa.selenium.WebDriver;
 import org.openqa.selenium.internal.WrapsDriver;
 import org.openqa.selenium.remote.SessionId;
@@ -84,6 +86,6 @@ protected WebDriver getUnwrappedDriver() {
     while (toReturn instanceof WrapsDriver) {
       toReturn = ((WrapsDriver) toReturn).getWrappedDriver();
     }
-    return toReturn;
+    return Preconditions.checkNotNull(toReturn);
   }
 }

java/server/src/org/openqa/selenium/remote/server/log/SessionLogsToFileRepository.java

@@ -94,10 +94,11 @@ public List<LogRecord> getLogRecords(SessionId sessionId) throws IOException {
     if (logFile == null) {
       return new ArrayList<LogRecord>();
     }
-    logFile.openLogReader();
-    ObjectInputStream logObjInStream = logFile.getLogReader();
+
     List<LogRecord> logRecords = new ArrayList<LogRecord>();
     try {
+      logFile.openLogReader();
+      ObjectInputStream logObjInStream = logFile.getLogReader();
       LogRecord tmpLogRecord;
       while (null != (tmpLogRecord = (LogRecord) logObjInStream
           .readObject())) {

java/server/src/org/openqa/selenium/server/ProxyHandler.java

@@ -16,6 +16,8 @@
 
 package org.openqa.selenium.server;
 
+import com.google.common.base.Preconditions;
+
 import cybervillains.ca.KeyStoreManager;
 
 import org.openqa.jetty.http.HttpConnection;
@@ -67,7 +69,7 @@
  * make proxy requests.
  * <p/>
  * The HttpTunnel mechanism is also used to implement the CONNECT method.
- * 
+ *
  * @author Greg Wilkins (gregw)
  * @author [email protected] (chained proxy)
  * @version $Id: ProxyHandler.java,v 1.34 2005/10/05 13:32:59 gregwilkins Exp $
@@ -641,7 +643,7 @@ protected void wireUpSslWithCyberVilliansCA(String host, SslRelay listener) {
           new KeyStoreManager(root, "http://127.0.0.1:" + port +
               "/selenium-server/sslSupport/blank_crl.pem");
       mgr.getCertificateByHostname(host);
-      mgr.getKeyStore().deleteEntry(KeyStoreManager._caPrivKeyAlias);
+      Preconditions.checkNotNull(mgr.getKeyStore()).deleteEntry(KeyStoreManager._caPrivKeyAlias);
       mgr.persist();
 
       listener.setKeystore(new File(root, "cybervillainsCA.jks").getAbsolutePath());
@@ -670,7 +672,7 @@ protected HttpTunnel newHttpTunnel(HttpResponse response, InetAddress iaddr,
 
   /**
    * Is URL Proxied. Method to allow derived handlers to select which URIs are proxied and to where.
-   * 
+   *
    * @param uri The requested URI, which should include a scheme, host and port.
    * @return The URL to proxy to, or null if the passed URI should not be proxied. The default
    *         implementation returns the passed uri if isForbidden() returns true.
@@ -688,7 +690,7 @@ protected URL isProxied(URI uri) throws MalformedURLException {
 
   /**
    * Is URL Forbidden.
-   * 
+   *
    * @return True if the URL is not forbidden. Calls isForbidden(scheme,host,port,true);
    */
   protected boolean isForbidden(URI uri) {
@@ -702,7 +704,7 @@ protected boolean isForbidden(URI uri) {
 
   /**
    * Is scheme,host & port Forbidden.
-   * 
+   *
    * @param scheme A scheme that mast be in the proxySchemes StringMap.
    * @param host A host that must pass the white and black lists
    * @return True if the request to the scheme,host and port is not forbidden.