diff options
| -rw-r--r-- | org.eclipse.jgit.test/tst/org/eclipse/jgit/api/SecurityManagerMissingPermissionsTest.java | 132 | ||||
| -rw-r--r-- | org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java | 41 |
2 files changed, 169 insertions, 4 deletions
diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/SecurityManagerMissingPermissionsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/api/SecurityManagerMissingPermissionsTest.java new file mode 100644 index 0000000000..a07f37009e --- /dev/null +++ b/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(); + } + } + } + +} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java b/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java index cd6647c626..91734a0c2b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/util/FS.java +++ b/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) { |