FS: Don't use innocuous threads for CompletableFuture

The default threads from the ForkJoinPool run without any privileges
when a SecurityManager is installed, leading to SecurityExceptions
when trying to create the probe file even if the application otherwise
has write privileges in the directory.

Use a dedicated ThreadPoolExecutor using daemon threads instead.

Bug: 551690
Change-Id: Id5376f09f0d7da5ceea367e1f0dfc70f823d62d3
Signed-off-by: Alex Jitianu <alex_jitianu@sync.ro>
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>

org.eclipse.jgit.test/tst/org/eclipse/jgit/api/SecurityManagerMissingPermissionsTest.java

@@ -0,0 +1,132 @@
+/*
+ * Copyright (c) 2019 Alex Jitianu <alex_jitianu@sync.ro> and others
+ *
+ * This program and the accompanying materials are made available under the
+ * terms of the Eclipse Distribution License v. 1.0 which is available at
+ * https://www.eclipse.org/org/documents/edl-v10.php.
+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+package org.eclipse.jgit.api;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.StringWriter;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.security.Policy;
+import java.util.Collections;
+
+import org.apache.log4j.Logger;
+import org.apache.log4j.PatternLayout;
+import org.apache.log4j.WriterAppender;
+import org.eclipse.jgit.junit.RepositoryTestCase;
+import org.eclipse.jgit.util.FileUtils;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Tests that using a SecurityManager does not result in errors logged.
+ */
+public class SecurityManagerMissingPermissionsTest extends RepositoryTestCase {
+
+	/**
+	 * Collects all logging sent to the logging system.
+	 */
+	private final StringWriter errorOutputWriter = new StringWriter();
+
+	/**
+	 * Appender to intercept all logging sent to the logging system.
+	 */
+	private WriterAppender appender;
+
+	private SecurityManager originalSecurityManager;
+
+	@Override
+	@Before
+	public void setUp() throws Exception {
+		originalSecurityManager = System.getSecurityManager();
+
+		appender = new WriterAppender(
+				new PatternLayout(PatternLayout.TTCC_CONVERSION_PATTERN),
+				errorOutputWriter);
+
+		Logger.getRootLogger().addAppender(appender);
+
+		refreshPolicyAllPermission(Policy.getPolicy());
+		System.setSecurityManager(new SecurityManager());
+		super.setUp();
+	}
+
+	/**
+	 * If a SecurityManager is active a lot of {@link java.io.FilePermission}
+	 * errors are thrown and logged while initializing a repository.
+	 *
+	 * @throws Exception
+	 */
+	@Test
+	public void testCreateNewRepos_MissingPermissions() throws Exception {
+		File wcTree = new File(getTemporaryDirectory(),
+				"CreateNewRepositoryTest_testCreateNewRepos");
+
+		File marker = new File(getTemporaryDirectory(), "marker");
+		Files.write(marker.toPath(), Collections.singletonList("Can write"));
+		assertTrue("Can write in test directory", marker.isFile());
+		FileUtils.delete(marker);
+		assertFalse("Can delete in test direcory", marker.exists());
+
+		Git git = Git.init().setBare(false)
+				.setDirectory(new File(wcTree.getAbsolutePath())).call();
+
+		addRepoToClose(git.getRepository());
+
+		assertEquals("", errorOutputWriter.toString());
+	}
+
+	@Override
+	@After
+	public void tearDown() throws Exception {
+		System.setSecurityManager(originalSecurityManager);
+		Logger.getRootLogger().removeAppender(appender);
+		super.tearDown();
+	}
+
+	/**
+	 * Refresh the Java Security Policy.
+	 *
+	 * @param policy
+	 *            the policy object
+	 *
+	 * @throws IOException
+	 *             if the temporary file that contains the policy could not be
+	 *             created
+	 */
+	private static void refreshPolicyAllPermission(Policy policy)
+			throws IOException {
+		// Starting with an all permissions policy.
+		String policyString = "grant { permission java.security.AllPermission; };";
+
+		// Do not use TemporaryFilesFactory, it will create a dependency cycle
+		Path policyFile = Files.createTempFile("testpolicy", ".txt");
+
+		try {
+			Files.write(policyFile, Collections.singletonList(policyString));
+			System.setProperty("java.security.policy",
+					policyFile.toUri().toURL().toString());
+			policy.refresh();
+		} finally {
+			try {
+				Files.delete(policyFile);
+			} catch (IOException e) {
+				// Do not log; the test tests for no logging having occurred
+				e.printStackTrace();
+			}
+		}
+	}
+
+}

org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2008, Shawn O. Pearce <spearce@spearce.org> and others
+ * Copyright (C) 2008, 2020 Shawn O. Pearce <spearce@spearce.org> and others
  *
  * This program and the accompanying materials are made available under the
  * terms of the Eclipse Distribution License v. 1.0 which is available at
@@ -49,11 +49,15 @@ import java.util.concurrent.CancellationException;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutionException;
+import java.util.concurrent.Executor;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.SynchronousQueue;
+import java.util.concurrent.ThreadPoolExecutor;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.concurrent.locks.Lock;
 import java.util.concurrent.locks.ReentrantLock;
@@ -221,6 +225,31 @@ public abstract class FS {
 		private static final Duration FALLBACK_MIN_RACY_INTERVAL = Duration
 				.ofMillis(10);
 
+		private static final AtomicInteger threadNumber = new AtomicInteger(1);
+
+		/**
+		 * Don't use the default thread factory of the ForkJoinPool for the
+		 * CompletableFuture; it runs without any privileges, which causes
+		 * trouble if a SecurityManager is present.
+		 * <p>
+		 * Instead use normal daemon threads. They'll belong to the
+		 * SecurityManager's thread group, or use the one of the calling thread,
+		 * as appropriate.
+		 * </p>
+		 *
+		 * @see java.util.concurrent.Executors#newCachedThreadPool()
+		 */
+		private static final Executor FUTURE_RUNNER = new ThreadPoolExecutor(0,
+				5, 30L, TimeUnit.SECONDS, new SynchronousQueue<Runnable>(),
+				runnable -> {
+					Thread t = new Thread(runnable, "FileStoreAttributeReader-" //$NON-NLS-1$
+							+ threadNumber.getAndIncrement());
+					// Make sure these threads don't prevent application/JVM
+					// shutdown.
+					t.setDaemon(true);
+					return t;
+				});
+
 		/**
 		 * Configures size and purge factor of the path-based cache for file
 		 * system attributes. Caching of file system attributes avoids recurring
@@ -304,8 +333,7 @@ public abstract class FS {
 								// Some earlier future might have set the value
 								// and removed itself since we checked for the
 								// value above. Hence check cache again.
-								FileStoreAttributes c = attributeCache
-										.get(s);
+								FileStoreAttributes c = attributeCache.get(s);
 								if (c != null) {
 									return Optional.of(c);
 								}
@@ -339,7 +367,7 @@ public abstract class FS {
 								locks.remove(s);
 							}
 							return attributes;
-						});
+						}, FUTURE_RUNNER);
 				f = f.exceptionally(e -> {
 					LOG.error(e.getLocalizedMessage(), e);
 					return Optional.empty();
@@ -450,6 +478,11 @@ public abstract class FS {
 				LOG.debug("{}: end measure timestamp resolution {} in {}", //$NON-NLS-1$
 						Thread.currentThread(), s, dir);
 				return Optional.of(fsResolution);
+			} catch (SecurityException e) {
+				// Log it here; most likely deleteProbe() below will also run
+				// into a SecurityException, and then this one will be lost
+				// without trace.
+				LOG.warn(e.getLocalizedMessage(), e);
 			} catch (AccessDeniedException e) {
 				LOG.warn(e.getLocalizedMessage(), e); // see bug 548648
 			} catch (IOException e) {