6244047: impossible to specify directories to logging FileHandler unless they exist

Reviewed-by: alanb

src/share/classes/java/util/logging/FileHandler.java

@@ -25,10 +25,19 @@
 
 package java.util.logging;
 
-import java.io.*;
+import static java.nio.file.StandardOpenOption.CREATE_NEW;
+import static java.nio.file.StandardOpenOption.WRITE;
+
+import java.io.BufferedOutputStream;
+import java.io.File;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.OutputStream;
 import java.nio.channels.FileChannel;
-import java.nio.channels.FileLock;
-import java.security.*;
+import java.nio.file.FileAlreadyExistsException;
+import java.nio.file.Paths;
+import java.security.AccessController;
+import java.security.PrivilegedAction;
 
 /**
  * Simple file logging <tt>Handler</tt>.
@@ -137,14 +146,16 @@ public class FileHandler extends StreamHandler {
     private int count;
     private String pattern;
     private String lockFileName;
-    private FileOutputStream lockStream;
+    private FileChannel lockFileChannel;
     private File files[];
     private static final int MAX_LOCKS = 100;
     private static java.util.HashMap<String, String> locks = new java.util.HashMap<>();
 
-    // A metered stream is a subclass of OutputStream that
-    //   (a) forwards all its output to a target stream
-    //   (b) keeps track of how many bytes have been written
+    /**
+     * A metered stream is a subclass of OutputStream that
+     * (a) forwards all its output to a target stream
+     * (b) keeps track of how many bytes have been written
+     */
     private class MeteredStream extends OutputStream {
         OutputStream out;
         int written;
@@ -189,9 +200,10 @@ public class FileHandler extends StreamHandler {
         setOutputStream(meter);
     }
 
-    // Private method to configure a FileHandler from LogManager
-    // properties and/or default values as specified in the class
-    // javadoc.
+    /**
+     * Configure a FileHandler from LogManager properties and/or default values
+     * as specified in the class javadoc.
+     */
     private void configure() {
         LogManager manager = LogManager.getLogManager();
 
@@ -287,7 +299,8 @@ public class FileHandler extends StreamHandler {
      *             the caller does not have <tt>LoggingPermission("control")</tt>.
      * @exception  IllegalArgumentException if pattern is an empty string
      */
-    public FileHandler(String pattern, boolean append) throws IOException, SecurityException {
+    public FileHandler(String pattern, boolean append) throws IOException,
+            SecurityException {
         if (pattern.length() < 1 ) {
             throw new IllegalArgumentException();
         }
@@ -376,8 +389,10 @@ public class FileHandler extends StreamHandler {
         openFiles();
     }
 
-    // Private method to open the set of output files, based on the
-    // configured instance variables.
+    /**
+     * Open the set of output files, based on the configured
+     * instance variables.
+     */
     private void openFiles() throws IOException {
         LogManager manager = LogManager.getLogManager();
         manager.checkPermission();
@@ -413,18 +428,18 @@ public class FileHandler extends StreamHandler {
                     // object.  Try again.
                     continue;
                 }
-                FileChannel fc;
+
                 try {
-                    lockStream = new FileOutputStream(lockFileName);
-                    fc = lockStream.getChannel();
-                } catch (IOException ix) {
-                    // We got an IOException while trying to open the file.
-                    // Try the next file.
+                    lockFileChannel = FileChannel.open(Paths.get(lockFileName),
+                            CREATE_NEW, WRITE);
+                } catch (FileAlreadyExistsException ix) {
+                    // try the next lock file name in the sequence
                     continue;
                 }
+
                 boolean available;
                 try {
-                    available = fc.tryLock() != null;
+                    available = lockFileChannel.tryLock() != null;
                     // We got the lock OK.
                 } catch (IOException ix) {
                     // We got an IOException while trying to get the lock.
@@ -440,7 +455,7 @@ public class FileHandler extends StreamHandler {
                 }
 
                 // We failed to get the lock.  Try next file.
-                fc.close();
+                lockFileChannel.close();
             }
         }
 
@@ -472,8 +487,17 @@ public class FileHandler extends StreamHandler {
         setErrorManager(new ErrorManager());
     }
 
-    // Generate a filename from a pattern.
-    private File generate(String pattern, int generation, int unique) throws IOException {
+    /**
+     * Generate a file based on a user-supplied pattern, generation number,
+     * and an integer uniqueness suffix
+     * @param pattern the pattern for naming the output file
+     * @param generation the generation number to distinguish rotated logs
+     * @param unique a unique number to resolve conflicts
+     * @return the generated File
+     * @throws IOException
+     */
+    private File generate(String pattern, int generation, int unique)
+            throws IOException {
         File file = null;
         String word = "";
         int ix = 0;
@@ -548,7 +572,9 @@ public class FileHandler extends StreamHandler {
         return file;
     }
 
-    // Rotate the set of output files
+    /**
+     * Rotate the set of output files
+     */
     private synchronized void rotate() {
         Level oldLevel = getLevel();
         setLevel(Level.OFF);
@@ -615,9 +641,8 @@ public class FileHandler extends StreamHandler {
             return;
         }
         try {
-            // Closing the lock file's FileOutputStream will close
-            // the underlying channel and free any locks.
-            lockStream.close();
+            // Close the lock file channel (which also will free any locks)
+            lockFileChannel.close();
         } catch (Exception ex) {
             // Problems closing the stream.  Punt.
         }
@@ -626,7 +651,7 @@ public class FileHandler extends StreamHandler {
         }
         new File(lockFileName).delete();
         lockFileName = null;
-        lockStream = null;
+        lockFileChannel = null;
     }
 
     private static class InitializationErrorManager extends ErrorManager {
@@ -636,6 +661,8 @@ public class FileHandler extends StreamHandler {
         }
     }
 
-    // Private native method to check if we are in a set UID program.
+    /**
+     * check if we are in a set UID program.
+     */
     private static native boolean isSetUID();
 }

test/java/util/logging/CheckLockLocationTest.java

+/*
+ * Copyright (c) 2012, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test
+ * @bug     6244047
+ * @author Jim Gish
+ * @summary throw more precise IOException when pattern specifies invalid directory
+ *
+ * @run  main/othervm CheckLockLocationTest
+ */
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.IOException;
+import java.nio.file.AccessDeniedException;
+import java.nio.file.FileSystemException;
+import java.nio.file.NoSuchFileException;
+import java.util.logging.FileHandler;
+public class CheckLockLocationTest {
+
+    private static final String NON_WRITABLE_DIR = "non-writable-dir";
+    private static final String NOT_A_DIR = "not-a-dir";
+    private static final String WRITABLE_DIR = "writable-dir";
+    private static final String NON_EXISTENT_DIR = "non-existent-dir";
+
+    public static void main(String... args) throws IOException {
+        // we'll base all file creation attempts on the system temp directory,
+        // %t and also try specifying non-existent directories and plain files
+        // that should be directories, and non-writable directories,
+        // to exercise all code paths of checking the lock location
+        File writableDir = setup();
+        // we now have three files/directories to work with:
+        //    writableDir
+        //    notAdir
+        //    nonWritableDir
+        //    nonExistentDir (which doesn't exist)
+        runTests(writableDir);
+    }
+
+    /**
+     * @param writableDir in which log and lock file are created
+     * @throws SecurityException
+     * @throws RuntimeException
+     * @throws IOException
+     */
+    private static void runTests(File writableDir) throws SecurityException,
+            RuntimeException, IOException {
+        // Test 1: make sure we can create FileHandler in writable directory
+        try {
+            new FileHandler("%t/" + WRITABLE_DIR + "/log.log");
+        } catch (IOException ex) {
+            throw new RuntimeException("Test failed: should have been able"
+                    + " to create FileHandler for " + "%t/" + WRITABLE_DIR
+                    + "/log.log in writable directory.", ex);
+        } finally {
+            // the above test leaves files in the directory.  Get rid of the
+            // files created and the directory
+            delete(writableDir);
+        }
+
+        // Test 2: creating FileHandler in non-writable directory should fail
+        try {
+            new FileHandler("%t/" + NON_WRITABLE_DIR + "/log.log");
+            throw new RuntimeException("Test failed: should not have been able"
+                    + " to create FileHandler for " + "%t/" + NON_WRITABLE_DIR
+                    + "/log.log in non-writable directory.");
+        } catch (IOException ex) {
+            // check for the right exception
+            if (!(ex instanceof AccessDeniedException)) {
+                throw new RuntimeException("Test failed: Expected exception was not an AccessDeniedException", ex);
+            }
+        }
+
+        // Test 3: creating FileHandler in non-directory should fail
+        try {
+            new FileHandler("%t/" + NOT_A_DIR + "/log.log");
+            throw new RuntimeException("Test failed: should not have been able"
+                    + " to create FileHandler for " + "%t/" + NOT_A_DIR
+                    + "/log.log in non-directory.");
+        } catch (IOException ex) {
+            // check for the right exception
+            if (!(ex instanceof FileSystemException && ex.getMessage().contains("Not a directory"))) {
+                throw new RuntimeException("Test failed: Expected exception was not a FileSystemException", ex);
+            }
+        }
+
+        // Test 4: make sure we can't create a FileHandler in a non-existent dir
+        try {
+            new FileHandler("%t/" + NON_EXISTENT_DIR + "/log.log");
+            throw new RuntimeException("Test failed: should not have been able"
+                    + " to create FileHandler for " + "%t/" + NON_EXISTENT_DIR
+                    + "/log.log in a non-existent directory.");
+        } catch (IOException ex) {
+            // check for the right exception
+            if (!(ex instanceof NoSuchFileException)) {
+                throw new RuntimeException("Test failed: Expected exception was not a NoSuchFileException", ex);
+            }
+        }
+    }
+
+    /**
+     * Setup all the files and directories needed for the tests
+     *
+     * @return writable directory created that needs to be deleted when done
+     * @throws RuntimeException
+     */
+    private static File setup() throws RuntimeException {
+        // First do some setup in the temporary directory (using same logic as
+        // FileHandler for %t pattern)
+        String tmpDir = System.getProperty("java.io.tmpdir"); // i.e. %t
+        if (tmpDir == null) {
+            tmpDir = System.getProperty("user.home");
+        }
+        File tmpOrHomeDir = new File(tmpDir);
+        // Create a writable directory here (%t/writable-dir)
+        File writableDir = new File(tmpOrHomeDir, WRITABLE_DIR);
+        if (!createFile(writableDir, true)) {
+            throw new RuntimeException("Test setup failed: unable to create"
+                    + " writable working directory "
+                    + writableDir.getAbsolutePath() );
+        }
+        // writableDirectory and its contents will be deleted after the test
+        // that uses it
+
+        // Create a plain file which we will attempt to use as a directory
+        // (%t/not-a-dir)
+        File notAdir = new File(tmpOrHomeDir, NOT_A_DIR);
+        if (!createFile(notAdir, false)) {
+            throw new RuntimeException("Test setup failed: unable to a plain"
+                    + " working file " + notAdir.getAbsolutePath() );
+        }
+        notAdir.deleteOnExit();
+
+        // Create a non-writable directory (%t/non-writable-dir)
+        File nonWritableDir = new File(tmpOrHomeDir, NON_WRITABLE_DIR);
+        if (!createFile(nonWritableDir, true)) {
+            throw new RuntimeException("Test setup failed: unable to create"
+                    + " a non-"
+                    + "writable working directory "
+                    + nonWritableDir.getAbsolutePath() );
+        }
+        nonWritableDir.deleteOnExit();
+
+        // make it non-writable
+        if (!nonWritableDir.setWritable(false)) {
+            throw new RuntimeException("Test setup failed: unable to make"
+                    + " working directory " + nonWritableDir.getAbsolutePath()
+                    + " non-writable.");
+        }
+
+        // make sure non-existent directory really doesn't exist
+        File nonExistentDir = new File(tmpOrHomeDir, NON_EXISTENT_DIR);
+        if (nonExistentDir.exists()) {
+            nonExistentDir.delete();
+        }
+        return writableDir;
+    }
+
+    /**
+     * @param newFile
+     * @return true if file already exists or creation succeeded
+     */
+    private static boolean createFile(File newFile, boolean makeDirectory) {
+        if (newFile.exists()) {
+            return true;
+        }
+        if (makeDirectory) {
+            return newFile.mkdir();
+        } else {
+            try {
+                return newFile.createNewFile();
+            } catch (IOException ioex) {
+                ioex.printStackTrace();
+                return false;
+            }
+        }
+    }
+
+    /*
+     * Recursively delete all files starting at specified file
+     */
+    private static void delete(File f) throws IOException {
+        if (f != null && f.isDirectory()) {
+            for (File c : f.listFiles())
+                delete(c);
+        }
+        if (!f.delete())
+            throw new FileNotFoundException("Failed to delete file: " + f);
+    }
+}