Set permission bits for "executable" attribute according to the umask

Bug: 424395
Change-Id: I3f5c55dd4c084529af2319029305ba2e174e0636
Signed-off-by: Andrey Loskutov <loskutov@gmx.de>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

3 files changed, 257 insertions, 3 deletions

diff --git a/org.eclipse.jgit.java7.test/src/org/eclipse/jgit/util/FSJava7Test.java b/org.eclipse.jgit.java7.test/src/org/eclipse/jgit/util/FSJava7Test.java
index 70eaef231a..91555f371a 100644
--- a/org.eclipse.jgit.java7.test/src/org/eclipse/jgit/util/FSJava7Test.java
+++ b/org.eclipse.jgit.java7.test/src/org/eclipse/jgit/util/FSJava7Test.java
@@ -46,13 +46,21 @@ 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;
+import java.util.Set;
 
 import org.eclipse.jgit.junit.RepositoryTestCase;
-import org.eclipse.jgit.util.FS;
-import org.eclipse.jgit.util.FileUtils;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -112,4 +120,70 @@ public class FSJava7Test {
 		assertTrue(fs.canExecute(target));
 	}
 
+	@Test
+	public void testExecutableAttributes() throws Exception {
+		FS fs = FS.DETECTED;
+		// If this assumption fails the test is halted and ignored.
+		assumeTrue(fs instanceof FS_POSIX_Java7);
+
+		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));
+		assertTrue(!permissions.contains(PosixFilePermission.OWNER_EXECUTE));
+
+		fs.setExecute(f, true);
+
+		permissions = readPermissions(f);
+		assertTrue("'owner' execute permission not set",
+				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));
+		}
+	}
+
+	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();
+	}
+
+	private Set<PosixFilePermission> readPermissions(File f) throws IOException {
+		return Files
+				.getFileAttributeView(f.toPath(), PosixFileAttributeView.class)
+				.readAttributes().permissions();
+	}
+
 }
diff --git a/org.eclipse.jgit.java7/src/org/eclipse/jgit/util/FS_POSIX_Java7.java b/org.eclipse.jgit.java7/src/org/eclipse/jgit/util/FS_POSIX_Java7.java
index 0879e8021f..4a73a9bcf5 100644
--- a/org.eclipse.jgit.java7/src/org/eclipse/jgit/util/FS_POSIX_Java7.java
+++ b/org.eclipse.jgit.java7/src/org/eclipse/jgit/util/FS_POSIX_Java7.java
@@ -43,14 +43,55 @@
 
 package org.eclipse.jgit.util;
 
+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.Path;
+import java.nio.file.attribute.PosixFilePermission;
+import java.util.Set;
 
 /**
  * FS implementation for Java7 on unix like systems
  */
 public class FS_POSIX_Java7 extends FS_POSIX {
 
+	/*
+	 * True if the current user "umask" allows to set execute bit for "others".
+	 * Can be null if "umask" is not supported (or returns unexpected values) by
+	 * current user shell.
+	 *
+	 * Bug 424395: with the umask of 0002 (user: rwx group: rwx others: rx) egit
+	 * checked out files as rwx,rwx,r (execution not allowed for "others"). To
+	 * fix this and properly set "executable" permission bit for "others", we
+	 * must consider the user umask on checkout
+	 */
+	private static final Boolean EXECUTE_FOR_OTHERS;
+
+	/*
+	 * True if the current user "umask" allows to set execute bit for "group".
+	 * Can be null if "umask" is not supported (or returns unexpected values) by
+	 * current user shell.
+	 */
+	private static final Boolean EXECUTE_FOR_GROUP;
+
+	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;
+		}
+	}
+
 	FS_POSIX_Java7(FS_POSIX_Java7 src) {
 		super(src);
 	}
@@ -76,7 +117,138 @@ public class FS_POSIX_Java7 extends FS_POSIX {
 
 	@Override
 	public boolean setExecute(File f, boolean canExecute) {
-		return FileUtil.setExecute(f, 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 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);
+	}
+
+	/**
+	 * 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).
+	 *
+	 * @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
+	 */
+	private 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$
+		}
+	}
+
+	/**
+	 * @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 static String readUmask() {
+		Process p;
+		try {
+			p = Runtime.getRuntime().exec(
+					new String[] { "sh", "-c", "umask" }, null, null); //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$
+			try (BufferedReader lineRead = new BufferedReader(
+					new InputStreamReader(p.getInputStream(), Charset
+							.defaultCharset().name()))) {
+				p.waitFor();
+				return lineRead.readLine();
+			}
+		} catch (Exception e) {
+			return null;
+		}
 	}
 
 	@Override
diff --git a/org.eclipse.jgit.java7/src/org/eclipse/jgit/util/FileUtil.java b/org.eclipse.jgit.java7/src/org/eclipse/jgit/util/FileUtil.java
index c958494d04..67fcc92633 100644
--- a/org.eclipse.jgit.java7/src/org/eclipse/jgit/util/FileUtil.java
+++ b/org.eclipse.jgit.java7/src/org/eclipse/jgit/util/FileUtil.java
@@ -152,6 +152,14 @@ class FileUtil {
 		return path.canExecute();
 	}
 
+	/**
+	 * @param path
+	 * @param executable
+	 * @return true if succeeded, false if not supported or failed
+	 * @deprecated the implementation is highly platform dependent, consider
+	 *             using {@link FS#setExecute(File, boolean)} instead
+	 */
+	@Deprecated
 	public static boolean setExecute(File path, boolean executable) {
 		if (!isFile(path))
 			return false;