From: Shawn Pearce <spearce@spearce.org>
Date: Sun, 10 May 2015 19:19:12 +0000 (-0700)
Subject: FS_POSIX: Rework umask detection to make it settable
X-Git-Tag: v4.0.0.201505191015-rc1~1^2~8^2
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=bfdd9630833a32856fc3b5396b17406192ea7634;p=jgit.git

FS_POSIX: Rework umask detection to make it settable

Avoid always calling `sh -c umask` on startup, instead deferring
the invocation until the first time a working tree file needs to
use the execute bit. This allows servers using bare repos to avoid
a costly fork+exec for a value that is never used.

Store the umask as an int instead of two Boolean. This is slightly
smaller memory (one int vs. two references) and makes it easier for
an application to force setting the umask to a value that overrides
whatever the shell told JGit.

Simplify the code to bail by returning early when canExecute is
false, which is the common case for working tree files.

Change-Id: Ie713647615bc5bdf5d71b731a6748c28ea21c900
---

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FSJava7Test.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FSJava7Test.java
index 9fba374dcd..e6a244e142 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FSJava7Test.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/util/FSJava7Test.java
@@ -46,15 +46,10 @@ package org.eclipse.jgit.util;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import static org.junit.Assume.assumeFalse;
-import static org.junit.Assume.assumeNotNull;
 import static org.junit.Assume.assumeTrue;
 
-import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
-import java.io.InputStreamReader;
-import java.nio.charset.Charset;
 import java.nio.file.Files;
 import java.nio.file.attribute.PosixFileAttributeView;
 import java.nio.file.attribute.PosixFilePermission;
@@ -123,29 +118,15 @@ public class FSJava7Test {
 
 	@Test
 	public void testExecutableAttributes() throws Exception {
-		FS fs = FS.DETECTED;
+		FS fs = FS.DETECTED.newInstance();
 		// If this assumption fails the test is halted and ignored.
 		assumeTrue(fs instanceof FS_POSIX);
+		((FS_POSIX) fs).setUmask(0022);
 
 		File f = new File(trash, "bla");
 		assertTrue(f.createNewFile());
 		assertFalse(fs.canExecute(f));
 
-		String umask = readUmask();
-		assumeNotNull(umask);
-
-		char others = umask.charAt(umask.length() - 1);
-
-		boolean badUmask;
-		if (others != '0' && others != '2' && others != '4' && others != '6') {
-			// umask is set in the way that "others" can not "execute" => git
-			// CLI will not set "execute" attribute for "others", so we also
-			// don't care
-			badUmask = true;
-		} else {
-			badUmask = false;
-		}
-
 		Set<PosixFilePermission> permissions = readPermissions(f);
 		assertTrue(!permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
 		assertTrue(!permissions.contains(PosixFilePermission.GROUP_EXECUTE));
@@ -158,27 +139,21 @@ public class FSJava7Test {
 				permissions.contains(PosixFilePermission.OWNER_EXECUTE));
 		assertTrue("'group' execute permission not set",
 				permissions.contains(PosixFilePermission.GROUP_EXECUTE));
-		if (badUmask) {
-			assertFalse("'others' execute permission set",
-					permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
-			System.err.println("WARNING: your system's umask: \"" + umask
-					+ "\" doesn't allow FSJava7Test to test if setting posix"
-					+ "  permissions for \"others\" works properly");
-			assumeFalse(badUmask);
-		} else {
-			assertTrue("'others' execute permission not set",
-					permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
-		}
-	}
+		assertTrue("'others' execute permission not set",
+				permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
+
+		((FS_POSIX) fs).setUmask(0033);
+		fs.setExecute(f, false);
+		assertFalse(fs.canExecute(f));
+		fs.setExecute(f, true);
 
-	private String readUmask() throws Exception {
-		Process p = Runtime.getRuntime().exec(
-				new String[] { "sh", "-c", "umask" }, null, null);
-		final BufferedReader lineRead = new BufferedReader(
-				new InputStreamReader(p.getInputStream(), Charset
-						.defaultCharset().name()));
-		p.waitFor();
-		return lineRead.readLine();
+		permissions = readPermissions(f);
+		assertTrue("'owner' execute permission not set",
+				permissions.contains(PosixFilePermission.OWNER_EXECUTE));
+		assertFalse("'group' execute permission set",
+				permissions.contains(PosixFilePermission.GROUP_EXECUTE));
+		assertFalse("'others' execute permission set",
+				permissions.contains(PosixFilePermission.OTHERS_EXECUTE));
 	}
 
 	private Set<PosixFilePermission> readPermissions(File f) throws IOException {
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java
index 47760f6618..9c92d7c4cd 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS_POSIX.java
@@ -66,140 +66,70 @@ import org.eclipse.jgit.lib.Repository;
  * @since 3.0
  */
 public class FS_POSIX extends FS {
+	private static final int DEFAULT_UMASK = 0022;
+	private volatile int umask = -1;
 
-	static {
-		String umask = readUmask();
-
-		// umask return value consists of 3 or 4 digits, like "002" or "0002"
-		if (umask != null && umask.length() > 0 && umask.matches("\\d{3,4}")) { //$NON-NLS-1$
-			EXECUTE_FOR_OTHERS = isGranted(PosixFilePermission.OTHERS_EXECUTE,
-					umask);
-			EXECUTE_FOR_GROUP = isGranted(PosixFilePermission.GROUP_EXECUTE,
-					umask);
-		} else {
-			EXECUTE_FOR_OTHERS = null;
-			EXECUTE_FOR_GROUP = null;
-		}
+	/** Default constructor. */
+	protected FS_POSIX() {
 	}
 
 	/**
-	 * @since 4.0
-	 */
-	protected static final Boolean EXECUTE_FOR_OTHERS;
-
-	/**
-	 * @since 4.0
+	 * Constructor
+	 *
+	 * @param src
+	 *            FS to copy some settings from
 	 */
-	protected static final Boolean EXECUTE_FOR_GROUP;
+	protected FS_POSIX(FS src) {
+		super(src);
+		if (src instanceof FS_POSIX) {
+			umask = ((FS_POSIX) src).umask;
+		}
+	}
 
 	@Override
 	public FS newInstance() {
-		return new FS_POSIX();
+		return new FS_POSIX(this);
 	}
 
 	/**
-	 * Derives requested permission from given octal umask value as defined e.g.
-	 * in <a href="http://linux.die.net/man/2/umask">http://linux.die.net/man/2/
-	 * umask</a>.
-	 * <p>
-	 * The umask expected here must consist of 3 or 4 digits. Last three digits
-	 * are significant here because they represent file permissions granted to
-	 * the "owner", "group" and "others" (in this order).
-	 * <p>
-	 * Each single digit from the umask represents 3 bits of the mask standing
-	 * for "<b>r</b>ead, <b>w</b>rite, e<b>x</b>ecute" permissions (in this
-	 * order).
-	 * <p>
-	 * The possible umask values table:
-	 *
-	 * <pre>
-	 * Value : Bits:Abbr.: Permission
-	 *     0 : 000 :rwx  : read, write and execute
-	 *     1 : 001 :rw   : read and write
-	 *     2 : 010 :rx   : read and execute
-	 *     3 : 011 :r    : read only
-	 *     4 : 100 :wx   : write and execute
-	 *     5 : 101 :w    : write only
-	 *     6 : 110 :x    : execute only
-	 *     7 : 111 :     : no permissions
-	 * </pre>
-	 * <p>
-	 * Note, that umask value is used to "mask" the requested permissions on
-	 * file creation by combining the requested permission bit with the
-	 * <b>negated</b> value of the umask bit.
-	 * <p>
-	 * Simply speaking, if a bit is <b>not</b> set in the umask, then the
-	 * appropriate right <b>will</b> be granted <b>if</b> requested. If a bit is
-	 * set in the umask value, then the appropriate permission will be not
-	 * granted.
-	 * <p>
-	 * Example:
-	 * <li>umask 023 ("000 010 011" or rwx rx r) combined with the request to
-	 * create an executable file with full set of permissions for everyone (777)
-	 * results in the file with permissions 754 (rwx rx r).
-	 * <li>umask 002 ("000 000 010" or rwx rwx rx) combined with the request to
-	 * create an executable file with full set of permissions for everyone (777)
-	 * results in the file with permissions 775 (rwx rwx rx).
-	 * <li>umask 002 ("000 000 010" or rwx rwx rx) combined with the request to
-	 * create a file without executable rights for everyone (666) results in the
-	 * file with permissions 664 (rw rw r).
+	 * Set the umask, overriding any value observed from the shell.
 	 *
-	 * @param p
-	 *            non null permission
 	 * @param umask
-	 *            octal umask value represented by at least three digits. The
-	 *            digits (read from the end to beginning of the umask) represent
-	 *            permissions for "others", "group" and "owner".
-	 *
-	 * @return true if the requested permission is set according to given umask
+	 *            mask to apply when creating files.
 	 * @since 4.0
 	 */
-	protected static Boolean isGranted(PosixFilePermission p, String umask) {
-		char val;
-		switch (p) {
-		case OTHERS_EXECUTE:
-			// Read last digit, because umask is ordered as: User/Group/Others.
-			val = umask.charAt(umask.length() - 1);
-			return isExecuteGranted(val);
-		case GROUP_EXECUTE:
-			val = umask.charAt(umask.length() - 2);
-			return isExecuteGranted(val);
-		default:
-			throw new UnsupportedOperationException(
-					"isGranted() for " + p + " is not implemented!"); //$NON-NLS-1$ //$NON-NLS-2$
-		}
+	public void setUmask(int umask) {
+		this.umask = umask;
 	}
 
-	/**
-	 * @param c
-	 *            character representing octal permission value from the table
-	 *            in {@link #isGranted(PosixFilePermission, String)}
-	 * @return true if the "execute" permission is granted according to given
-	 *         character
-	 */
-	private static Boolean isExecuteGranted(char c) {
-		if (c == '0' || c == '2' || c == '4' || c == '6')
-			return Boolean.TRUE;
-		return Boolean.FALSE;
+	private int umask() {
+		int u = umask;
+		if (u == -1) {
+			u = readUmask();
+			umask = u;
+		}
+		return u;
 	}
 
-	/**
-	 * @return umask returned from running umask command in a shell
-	 * @since 4.0
-	 */
-	protected static String readUmask() {
-		Process p;
+	/** @return mask returned from running {@code umask} command in shell. */
+	private static int readUmask() {
 		try {
-			p = Runtime.getRuntime().exec(
-					new String[] { "sh", "-c", "umask" }, null, null); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+			Process p = Runtime.getRuntime().exec(
+					new String[] { "sh", "-c", "umask" }, //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+					null, null);
 			try (BufferedReader lineRead = new BufferedReader(
 					new InputStreamReader(p.getInputStream(), Charset
 							.defaultCharset().name()))) {
-				p.waitFor();
-				return lineRead.readLine();
+				if (p.waitFor() == 0) {
+					String s = lineRead.readLine();
+					if (s.matches("0?\\d{3}")) { //$NON-NLS-1$
+						return Integer.parseInt(s, 8);
+					}
+				}
+				return DEFAULT_UMASK;
 			}
 		} catch (Exception e) {
-			return null;
+			return DEFAULT_UMASK;
 		}
 	}
 
@@ -229,23 +159,6 @@ public class FS_POSIX extends FS {
 		return null;
 	}
 
-	/**
-	 * Default constructor
-	 */
-	protected FS_POSIX() {
-		super();
-	}
-
-	/**
-	 * Constructor
-	 *
-	 * @param src
-	 *            FS to copy some settings from
-	 */
-	protected FS_POSIX(FS src) {
-		super(src);
-	}
-
 	@Override
 	public boolean isCaseSensitive() {
 		return !SystemReader.getInstance().isMacOS();
@@ -265,35 +178,40 @@ public class FS_POSIX extends FS {
 	public boolean setExecute(File f, boolean canExecute) {
 		if (!isFile(f))
 			return false;
-		// only if the execute has to be set, and we know the umask
-		if (canExecute && EXECUTE_FOR_OTHERS != null) {
-			try {
-				Path path = f.toPath();
-				Set<PosixFilePermission> pset = Files
-						.getPosixFilePermissions(path);
-				// user is always allowed to set execute
-				pset.add(PosixFilePermission.OWNER_EXECUTE);
-
-				if (EXECUTE_FOR_GROUP.booleanValue())
-					pset.add(PosixFilePermission.GROUP_EXECUTE);
-
-				if (EXECUTE_FOR_OTHERS.booleanValue())
-					pset.add(PosixFilePermission.OTHERS_EXECUTE);
-
-				Files.setPosixFilePermissions(path, pset);
-				return true;
-			} catch (IOException e) {
-				// The interface doesn't allow to throw IOException
-				final boolean debug = Boolean.parseBoolean(SystemReader
-						.getInstance().getProperty("jgit.fs.debug")); //$NON-NLS-1$
-				if (debug)
-					System.err.println(e);
-				return false;
-			}
+		if (!canExecute)
+			return f.setExecutable(false);
+
+		try {
+			Path path = f.toPath();
+			Set<PosixFilePermission> pset = Files.getPosixFilePermissions(path);
+
+			// owner (user) is always allowed to execute.
+			pset.add(PosixFilePermission.OWNER_EXECUTE);
+
+			int mask = umask();
+			apply(pset, mask, PosixFilePermission.GROUP_EXECUTE, 1 << 3);
+			apply(pset, mask, PosixFilePermission.OTHERS_EXECUTE, 1);
+			Files.setPosixFilePermissions(path, pset);
+			return true;
+		} catch (IOException e) {
+			// The interface doesn't allow to throw IOException
+			final boolean debug = Boolean.parseBoolean(SystemReader
+					.getInstance().getProperty("jgit.fs.debug")); //$NON-NLS-1$
+			if (debug)
+				System.err.println(e);
+			return false;
+		}
+	}
+
+	private static void apply(Set<PosixFilePermission> set,
+			int umask, PosixFilePermission perm, int test) {
+		if ((umask & test) == 0) {
+			// If bit is clear in umask, permission is allowed.
+			set.add(perm);
+		} else {
+			// If bit is set in umask, permission is denied.
+			set.remove(perm);
 		}
-		// if umask is not working for some reason: fall back to default (buggy)
-		// implementation which does not consider umask: see bug 424395
-		return f.setExecutable(canExecute);
 	}
 
 	@Override