Index: conf/taskcontroller.cfg =================================================================== --- conf/taskcontroller.cfg (revision 790848) +++ conf/taskcontroller.cfg (working copy) @@ -1,3 +1,2 @@ -mapred.local.dir=#configured value of hadoop.tmp.dir it can be a list of paths comma seperated -hadoop.pid.dir=#configured HADOOP_PID_DIR -hadoop.indent.str=#configured HADOOP_IDENT_STR +mapred.local.dir=#configured value of mapred.local.dir. It can be a list of comma separated paths. +hadoop.log.dir=#configured value of hadoop.log.dir. \ No newline at end of file Index: src/test/core/org/apache/hadoop/fs/TestLocalDirAllocator.java =================================================================== --- src/test/core/org/apache/hadoop/fs/TestLocalDirAllocator.java (revision 790848) +++ src/test/core/org/apache/hadoop/fs/TestLocalDirAllocator.java (working copy) @@ -19,8 +19,11 @@ import java.io.File; import java.io.IOException; +import java.io.OutputStream; import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.util.DiskChecker; +import org.apache.hadoop.util.DiskChecker.PermissionsInfo; import org.apache.hadoop.util.Shell; import junit.framework.TestCase; @@ -86,7 +89,15 @@ result.delete(); return result; } - + + private File createTempFile(long size, PermissionsInfo perms) + throws IOException { + File result = + dirAllocator.createTmpFileForWrite(FILENAME, size, conf, perms); + //result.delete(); + return result; + } + /** Two buffer dirs. The first dir does not exist & is on a read-only disk; * The second dir exists & is RW * @throws Exception @@ -207,5 +218,47 @@ rmBufferDirs(); } } - + + /** + * Test directory creation with secure permissions. + * + * @throws IOException + */ + public void testSecureFileCreation() + throws IOException { + if (isWindows) + return; + try { + conf.set(CONTEXT, BUFFER_DIR[5]); + Path dir = createAndAssertSecurePath("tmp1/tmp2/tmp3/tmp4/tmp5/job.xml"); + Path file = new Path(dir.getParent(), "job.xml"); + OutputStream out = dir.getFileSystem(conf).create(file); + DiskChecker.setPermissions(new File(file.toUri().getPath()), + DiskChecker.sevenZeroZero); + + // Just write something to the file + try { + conf.writeXml(out); + } finally { + out.close(); + } + // permissions should not change + File f = new File(file.getParent().toUri().getPath()); + assertTrue(f.canRead()); + assertTrue(f.canWrite()); + assertTrue(f.canExecute()); + } finally { + rmBufferDirs(); + } + } + + private Path createAndAssertSecurePath(String pa) + throws IOException { + Path path = dirAllocator.getPrivateLocalPathForWrite(pa, -1, conf); + File dir = new File(path.getParent().toUri().getPath()); + assertTrue(dir.canRead()); + assertTrue(dir.canWrite()); + assertTrue(dir.canExecute()); + return path; + } } Index: src/java/org/apache/hadoop/fs/LocalDirAllocator.java =================================================================== --- src/java/org/apache/hadoop/fs/LocalDirAllocator.java (revision 790848) +++ src/java/org/apache/hadoop/fs/LocalDirAllocator.java (working copy) @@ -27,6 +27,7 @@ import org.apache.hadoop.fs.FileSystem; import org.apache.hadoop.fs.Path; import org.apache.hadoop.util.DiskChecker.DiskErrorException; +import org.apache.hadoop.util.DiskChecker.PermissionsInfo; import org.apache.hadoop.conf.Configuration; /** An implementation of a round-robin scheme for disk allocation for creating @@ -61,7 +62,10 @@ * parent for all file write/read allocations. */ public class LocalDirAllocator { - + + final static Log LOG = + LogFactory.getLog(LocalDirAllocator.class); + //A Map from the config item names like "mapred.local.dir", //"dfs.client.buffer.dir" to the instance of the AllocatorPerContext. This //is a static object to make sure there exists exactly one instance per JVM @@ -125,9 +129,64 @@ public Path getLocalPathForWrite(String pathStr, long size, Configuration conf) throws IOException { AllocatorPerContext context = obtainContext(contextCfgItemName); - return context.getLocalPathForWrite(pathStr, size, conf); + return context.getLocalPathForWrite(pathStr, size, conf, null); } - + + /** + * Get a path from the local FS. Pass size as -1 if not known apriori. We + * round-robin over the set of disks (via the configured dirs) and return the + * first complete path which has enough space. Also set the given permissions + * to all or the last of the newly created directories using permissions + * information passed. + * + * @param pathStr the requested path (this will be created on the first + * available disk) + * @param size + * @param conf + * @param perm + * @return path to write in + * @throws IOException + */ + public Path getLocalPathForWrite(String pathStr, long size, + Configuration conf, PermissionsInfo perm) + throws IOException { + AllocatorPerContext context = obtainContext(contextCfgItemName); + return context.getLocalPathForWrite(pathStr, size, conf, perm); + } + + /** + * Get a secure local path for writing. All the newly created directories are + * set to be accessible only by the caller. + * + * @param pathStr + * @param conf + * @return + * @throws IOException + */ + public Path getPrivateLocalPathForWrite(String pathStr, Configuration conf) + throws IOException { + return getPrivateLocalPathForWrite(pathStr, -1, conf); + } + + /** + * Get a secure local path for writing. All the newly created directories are + * set to be accessible only by the calle + * + * @param pathStr + * @param size + * @param conf + * @return + * @throws IOException + */ + public Path getPrivateLocalPathForWrite(String pathStr, long size, + Configuration conf) + throws IOException { + LOG.debug("Getting a secure path for " + pathStr); + AllocatorPerContext context = obtainContext(contextCfgItemName); + return context.getLocalPathForWrite(pathStr, size, conf, + DiskChecker.sevenZeroZero); + } + /** Get a path from the local FS for reading. We search through all the * configured dirs for the file's existence and return the complete * path to the file when we find one @@ -155,10 +214,31 @@ */ public File createTmpFileForWrite(String pathStr, long size, Configuration conf) throws IOException { + return createTmpFileForWrite(pathStr, size, conf, null); + } + + /** + * Creates a temporary file in the local FS. Pass size as -1 if not known + * apriori. We round-robin over the set of disks (via the configured dirs) and + * select the first complete path which has enough space. All or the last of + * the newly created directories will be set permissions using the permission + * information passed. A file is created on this directory. The file is + * guaranteed to go away when the JVM exits. + * + * @param pathStr + * @param size + * @param conf + * @param permis + * @return + * @throws IOException + */ + public File createTmpFileForWrite(String pathStr, long size, + Configuration conf, PermissionsInfo permis) + throws IOException { AllocatorPerContext context = obtainContext(contextCfgItemName); - return context.createTmpFileForWrite(pathStr, size, conf); + return context.createTmpFileForWrite(pathStr, size, conf, permis); } - + /** Method to check whether a context is valid * @param contextCfgItemName * @return true/false @@ -192,9 +272,6 @@ private static class AllocatorPerContext { - private final Log LOG = - LogFactory.getLog(AllocatorPerContext.class); - private int dirNumLastAccessed; private Random dirIndexRandomizer = new Random(); private FileSystem localFS; @@ -210,7 +287,9 @@ /** This method gets called everytime before any read/write to make sure * that any change to localDirs is reflected immediately. */ - private void confChanged(Configuration conf) throws IOException { + private void confChanged(Configuration conf, + PermissionsInfo perm) + throws IOException { String newLocalDirs = conf.get(contextCfgItemName); if (!newLocalDirs.equals(savedLocalDirs)) { localDirs = conf.getStrings(contextCfgItemName); @@ -221,24 +300,24 @@ for (int i = 0; i < numDirs; i++) { try { // filter problematic directories - Path tmpDir = new Path(localDirs[i]); - if(localFS.mkdirs(tmpDir)|| localFS.exists(tmpDir)) { - try { - DiskChecker.checkDir(new File(localDirs[i])); - dirs.add(localDirs[i]); - dfList.add(new DF(new File(localDirs[i]), 30000)); - } catch (DiskErrorException de) { - LOG.warn( localDirs[i] + "is not writable\n" + - StringUtils.stringifyException(de)); - } - } else { - LOG.warn( "Failed to create " + localDirs[i]); + try { + DiskChecker.checkDir(new File(localDirs[i]), perm); + dirs.add(localDirs[i]); + dfList.add(new DF(new File(localDirs[i]), 30000)); + } catch (DiskErrorException de) { + LOG.warn(localDirs[i] + "is not writable\n" + + StringUtils.stringifyException(de)); } } catch (IOException ie) { LOG.warn( "Failed to create " + localDirs[i] + ": " + ie.getMessage() + "\n" + StringUtils.stringifyException(ie)); } //ignore } + + if (dirs.size() == 0) { + throw new IOException( + "None of the local directories configured are writable."); + } localDirs = dirs.toArray(new String[dirs.size()]); dirDF = dfList.toArray(new DF[dirs.size()]); savedLocalDirs = newLocalDirs; @@ -248,13 +327,14 @@ } } - private Path createPath(String path) throws IOException { + private Path createPath(String path, PermissionsInfo perm) { Path file = new Path(new Path(localDirs[dirNumLastAccessed]), path); //check whether we are able to create a directory here. If the disk //happens to be RDONLY we will fail try { - DiskChecker.checkDir(new File(file.getParent().toUri().getPath())); + DiskChecker.checkDir(new File(file.getParent().toUri().getPath()), + perm); return file; } catch (DiskErrorException d) { LOG.warn(StringUtils.stringifyException(d)); @@ -278,7 +358,7 @@ */ public synchronized Path getLocalPathForWrite(String path, Configuration conf) throws IOException { - return getLocalPathForWrite(path, SIZE_UNKNOWN, conf); + return getLocalPathForWrite(path, SIZE_UNKNOWN, conf, null); } /** Get a path from the local FS. If size is known, we go @@ -287,10 +367,16 @@ * * If size is not known, use roulette selection -- pick directories * with probability proportional to their available space. + * @param pathStr + * @param size + * @param conf + * @param perm + * @return + * @throws IOException */ public synchronized Path getLocalPathForWrite(String pathStr, long size, - Configuration conf) throws IOException { - confChanged(conf); + Configuration conf, PermissionsInfo perm) throws IOException { + confChanged(conf, perm); int numDirs = localDirs.length; int numDirsSearched = 0; //remove the leading slash from the path (to make sure that the uri @@ -321,7 +407,7 @@ dir++; } dirNumLastAccessed = dir; - returnPath = createPath(pathStr); + returnPath = createPath(pathStr, perm); if (returnPath == null) { totalAvailable -= availableOnDisk[dir]; availableOnDisk[dir] = 0; // skip this disk @@ -332,7 +418,7 @@ while (numDirsSearched < numDirs && returnPath == null) { long capacity = dirDF[dirNumLastAccessed].getAvailable(); if (capacity > size) { - returnPath = createPath(pathStr); + returnPath = createPath(pathStr, perm); } dirNumLastAccessed++; dirNumLastAccessed = dirNumLastAccessed % numDirs; @@ -353,12 +439,36 @@ * round-robin over the set of disks (via the configured dirs) and return * a file on the first path which has enough space. The file is guaranteed * to go away when the JVM exits. + * @param pathStr + * @param size + * @param conf + * @return + * @throws IOException */ public File createTmpFileForWrite(String pathStr, long size, Configuration conf) throws IOException { + return createTmpFileForWrite(pathStr, size, conf, null); + } + /** + * Creates a file on the local FS. Pass size as -1 if not known apriori. We + * round-robin over the set of disks (via the configured dirs) and return a + * file on the first path which has enough space. The file is guaranteed to + * go away when the JVM exits. Also set the given permissions to all or the + * last of the newly created directories using permissions information + * passed. + * + * @param pathStr + * @param size + * @param conf + * @return + * @throws IOException + */ + public File createTmpFileForWrite(String pathStr, long size, + Configuration conf, PermissionsInfo permis) + throws IOException { // find an appropriate directory - Path path = getLocalPathForWrite(pathStr, size, conf); + Path path = getLocalPathForWrite(pathStr, size, conf, permis); File dir = new File(path.getParent().toUri().getPath()); String prefix = path.getName(); @@ -374,7 +484,7 @@ */ public synchronized Path getLocalPathToRead(String pathStr, Configuration conf) throws IOException { - confChanged(conf); + confChanged(conf, null); int numDirs = localDirs.length; int numDirsSearched = 0; //remove the leading slash from the path (to make sure that the uri Index: src/java/org/apache/hadoop/util/DiskChecker.java =================================================================== --- src/java/org/apache/hadoop/util/DiskChecker.java (revision 790848) +++ src/java/org/apache/hadoop/util/DiskChecker.java (working copy) @@ -21,12 +21,19 @@ import java.io.File; import java.io.IOException; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.apache.hadoop.fs.FileUtil; + /** * Class that provides utility functions for checking disk problem */ public class DiskChecker { + private final static Log LOG = + LogFactory.getLog(DiskChecker.class); + public static class DiskErrorException extends IOException { public DiskErrorException(String msg) { super(msg); @@ -53,37 +60,204 @@ * @return true on success, false on failure */ public static boolean mkdirsWithExistsCheck(File dir) { - if (dir.mkdir() || dir.exists()) { + return mkdirsWithExistsCheck(dir, null); + } + + /** + * The semantics of mkdirsWithExistsCheck method is different from the mkdirs + * method provided in the Sun's java.io.File class in the following way: While + * creating the non-existent parent directories, this method checks for the + * existence of those directories if the mkdir fails at any point (since that + * directory might have just been created by some other process). If both + * mkdir() and the exists() check fails for any seemingly non-existent + * directory, then we signal an error; Sun's mkdir would signal an error + * (return false) if a directory it is attempting to create already exists or + * the mkdir fails. + * + * Using the permissions information passed, the leaf of the newly + * created directories is set appropriate permissions. + * + * @param dir + * @param dirCreationPermisInfo + * @return true on success, false on failure + */ + public static boolean mkdirsWithExistsCheck(File dir, + PermissionsInfo dirCreationPermisInfo) { + if (dir.exists()) { + LOG.debug(dir + " exists, not setting permissions. Returning"); return true; + } else if (dir.mkdir()) { + LOG.debug(dir + " creating, setting permissions and returning"); + return setPermissions(dir, dirCreationPermisInfo); } + File canonDir = null; try { canonDir = dir.getCanonicalFile(); } catch (IOException e) { return false; } + String parent = canonDir.getParent(); - return (parent != null) && - (mkdirsWithExistsCheck(new File(parent)) && - (canonDir.mkdir() || canonDir.exists())); + + boolean parentCreation = + (parent != null) + && mkdirsWithExistsCheck(new File(parent), dirCreationPermisInfo); + LOG.debug("Status for creation of the parent " + parent + " : " + " " + + parentCreation); + boolean canonDirCreation = + parentCreation && (canonDir.mkdir() || canonDir.exists()); + LOG.debug("Status for creation of the directory " + canonDir + " : " + " " + + canonDirCreation); + boolean canonDirPermissions = + canonDirCreation && setPermissions(canonDir, dirCreationPermisInfo); + LOG.debug("Status for setting permissions for the directory " + canonDir + + " " + canonDirPermissions); + return canonDirPermissions; } - + + /** + * Completely open permissions + */ + public static PermissionsInfo sevenSevenSeven = + new PermissionsInfo(true, true, true, false, false, false); + + /** + * Completely private permissions + */ + public static PermissionsInfo sevenZeroZero = + new PermissionsInfo(true, true, true, true, true, true); + + /** + * Permissions rwxr_xr_x + */ + public static PermissionsInfo sevenFiveFive = + new PermissionsInfo(true, true, true, false, true, false); + + /** + * Set permission on the given file path using the specified permissions + * information. We use java api to set permission instead of spawning chmod + * processes. This saves a lot of time. Using this, one can set all possible + * combinations of permissions for the owner of the file. But permissions for + * the group and all others can only be set together, i.e. permissions for + * group cannot be set different from those for others and vice versa. + * + * This method should satisfy the needs of most of the applications. For those + * it doesn't, {@link FileUtil#chmod(String, String)} can be used. + * + * @param f file path + * @param pInfo permissions information + * @return true if success, false otherwise + */ + public static boolean setPermissions(File f, PermissionsInfo pInfo) { + if (pInfo == null) { + LOG.debug(" PermissionsInfo is null, returning."); + return true; + } + + LOG.debug("Setting permission for " + f.getAbsolutePath()); + + // Clear all the flags + f.setReadable(false, false); + f.setWritable(false, false); + f.setExecutable(false, false); + + boolean ret = true; + ret = f.setReadable(pInfo.readPermissions, pInfo.readPermsOwnerOnly); + LOG.debug("Readable status for " + f + " set to " + ret); + ret = + f.setWritable(pInfo.writePermissions, pInfo.writePermsOwnerOnly) + && ret; + LOG.debug("Writable status for " + f + " set to " + ret); + ret = + f.setExecutable(pInfo.executablePermissions, + pInfo.executePermsOwnerOnly) + && ret; + + LOG.debug("Executable status for " + f + " set to " + ret); + return ret; + } + + /** + * Tries creating the given directory, and if needed its ancestors in the file + * system hierarchy. + * + * @param dir + * @throws DiskErrorException + */ public static void checkDir(File dir) throws DiskErrorException { - if (!mkdirsWithExistsCheck(dir)) - throw new DiskErrorException("can not create directory: " - + dir.toString()); - - if (!dir.isDirectory()) - throw new DiskErrorException("not a directory: " - + dir.toString()); - - if (!dir.canRead()) - throw new DiskErrorException("directory is not readable: " - + dir.toString()); - - if (!dir.canWrite()) - throw new DiskErrorException("directory is not writable: " - + dir.toString()); + checkDir(dir, null); } + /** + * Tries creating the given directory, and if needed its ancestors in the file + * system hierarchy. Depending on the permission information passed, sets + * permissions to the leaf of newly created paths. + * + * @param file + * @param fileCreationPermissionsInfo + * @throws DiskErrorException + */ + public static void checkDir(File file, + PermissionsInfo fileCreationPermissionsInfo) + throws DiskErrorException { + + if (!mkdirsWithExistsCheck(file, fileCreationPermissionsInfo)) { + throw new DiskErrorException("can not create directory: " + + file.toString()); + } + + if (!file.isDirectory() && !setPermissions(file, fileCreationPermissionsInfo)) { + throw new DiskErrorException("Cannot set permissions to directory: " + + file.toString()); + } + + if (!file.isDirectory()) + throw new DiskErrorException("not a directory: " + file.toString()); + + if (!file.canRead()) + throw new DiskErrorException("directory is not readable: " + + file.toString()); + + if (!file.canWrite()) + throw new DiskErrorException("directory is not writable: " + + file.toString()); + } + + /** + * Permission information useful for setting all or the leaf node for a given + * path. Using this, one can set all possible combinations of permissions for + * the owner of the file. But permissions for the group and all others can + * only be set together, i.e. permissions for group cannot be set different + * from those for others and vice versa. + */ + public static class PermissionsInfo { + boolean readPermissions; + boolean writePermissions; + boolean executablePermissions; + boolean readPermsOwnerOnly; + boolean writePermsOwnerOnly; + boolean executePermsOwnerOnly; + + /** + * Create a permissions-info object with the given attributes + * + * @param readPerms + * @param writePerms + * @param executePerms + * @param readOwnerOnly + * @param writeOwnerOnly + * @param executeOwnerOnly + */ + public PermissionsInfo(boolean readPerms, boolean writePerms, + boolean executePerms, boolean readOwnerOnly, boolean writeOwnerOnly, + boolean executeOwnerOnly) { + readPermissions = readPerms; + writePermissions = writePerms; + executablePermissions = executePerms; + readPermsOwnerOnly = readOwnerOnly; + writePermsOwnerOnly = writeOwnerOnly; + executePermsOwnerOnly = executeOwnerOnly; + } + } }