From dc56e38f125f0197862b5f3226b030b910a1ef1d Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Sun, 30 Apr 2017 00:15:44 +0200 Subject: [PATCH 1/6] Using guava convenience functions for simple exception propagation rules --- .../java/org/cryptomator/cryptofs/UncheckedThrows.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/UncheckedThrows.java b/src/main/java/org/cryptomator/cryptofs/UncheckedThrows.java index 16db3a9c..2149168f 100644 --- a/src/main/java/org/cryptomator/cryptofs/UncheckedThrows.java +++ b/src/main/java/org/cryptomator/cryptofs/UncheckedThrows.java @@ -6,6 +6,8 @@ import java.util.LinkedList; import java.util.function.Supplier; +import com.google.common.base.Throwables; + /** *

* Implements means to throw checked exceptions crossing method boundaries which do not declare them. @@ -35,9 +37,7 @@ public T from(Supplier action) throws E { allowedToBeThrownUnchecked.addFirst(type); return action.get(); } catch (ExceptionThrownUnchecked e) { - if (type.isInstance(e.getCause())) { - throw type.cast(e.getCause()); - } + Throwables.throwIfInstanceOf(e.getCause(), type); throw e; } finally { allowedToBeThrownUnchecked.removeFirst(); @@ -56,9 +56,8 @@ public static RethrowUncheckedWithoutAction rethrowUnch public T from(SupplierThrowingException action) { try { return action.get(); - } catch (RuntimeException e) { - throw e; // don't catch unchecked exceptions } catch (Exception e) { + Throwables.throwIfUnchecked(e); // don't catch unchecked exceptions throw new ExceptionThrownUnchecked(e); } } From 9135c0c320673f4b7c30c594f9df943a3bb6f285 Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Fri, 26 May 2017 13:48:30 +0200 Subject: [PATCH 2/6] moved dagger-compiler as suggested on github.com/google/dagger --- pom.xml | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/pom.xml b/pom.xml index a58be596..5271efaa 100644 --- a/pom.xml +++ b/pom.xml @@ -91,12 +91,6 @@ dagger ${dagger.version} - - com.google.dagger - dagger-compiler - ${dagger.version} - provided - @@ -154,6 +148,13 @@ ${java.version} ${java.version} true + + + com.google.dagger + dagger-compiler + ${dagger.version} + + From 7e8665d8ebb632abea0aa123fcb5fc6b2c70fa0a Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 30 May 2017 10:52:19 +0200 Subject: [PATCH 3/6] fixed int overflow --- src/main/java/org/cryptomator/cryptofs/OpenCryptoFile.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/OpenCryptoFile.java index c2c705f3..d22dad86 100644 --- a/src/main/java/org/cryptomator/cryptofs/OpenCryptoFile.java +++ b/src/main/java/org/cryptomator/cryptofs/OpenCryptoFile.java @@ -110,9 +110,9 @@ private void write(ByteSource source, long position) throws IOException { int written = 0; while (source.hasRemaining()) { long currentPosition = position + written; - long chunkIndex = currentPosition / cleartextChunkSize; - int offsetInChunk = (int) currentPosition % cleartextChunkSize; - int len = (int) min(source.remaining(), cleartextChunkSize - offsetInChunk); + long chunkIndex = currentPosition / cleartextChunkSize; // floor by int-truncation + int offsetInChunk = (int) (currentPosition % cleartextChunkSize); // known to fit in int, because cleartextChunkSize is int + int len = (int) min(source.remaining(), cleartextChunkSize - offsetInChunk); // known to fit in int, because second argument is int long minSize = currentPosition + len; size.getAndUpdate(size -> max(minSize, size)); if (len == cleartextChunkSize) { From 2387356e33acdfa2d5ea29188bf9e888719fa19e Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 30 May 2017 11:47:36 +0200 Subject: [PATCH 4/6] Single I/O operation in DirectoryIdLoader --- .../cryptofs/DirectoryIdLoader.java | 25 +++++-- .../cryptofs/DirectoryIdLoaderTest.java | 65 ++++++++++++++----- 2 files changed, 68 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java b/src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java index a1b3fcda..9b567580 100644 --- a/src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java +++ b/src/main/java/org/cryptomator/cryptofs/DirectoryIdLoader.java @@ -1,9 +1,12 @@ package org.cryptomator.cryptofs; import java.io.IOException; +import java.nio.ByteBuffer; +import java.nio.channels.FileChannel; import java.nio.charset.StandardCharsets; -import java.nio.file.Files; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.nio.file.StandardOpenOption; import java.util.UUID; import javax.inject.Inject; @@ -13,19 +16,29 @@ @PerFileSystem class DirectoryIdLoader extends CacheLoader { + private static final int MAX_DIR_ID_LENGTH = 1000; + @Inject public DirectoryIdLoader() { } @Override public String load(Path dirFilePath) throws IOException { - if (Files.exists(dirFilePath)) { - byte[] bytes = Files.readAllBytes(dirFilePath); - if (bytes.length == 0) { + try (FileChannel ch = FileChannel.open(dirFilePath, StandardOpenOption.READ)) { + long size = ch.size(); + if (size == 0) { throw new IOException("Invalid, empty directory file: " + dirFilePath); + } else if (size > MAX_DIR_ID_LENGTH) { + throw new IOException("Unexpectedly large directory file: " + dirFilePath); + } else { + assert size <= MAX_DIR_ID_LENGTH; // thus int + ByteBuffer buffer = ByteBuffer.allocate((int) size); + int read = ch.read(buffer); + assert read == size; + buffer.flip(); + return StandardCharsets.UTF_8.decode(buffer).toString(); } - return new String(bytes, StandardCharsets.UTF_8); - } else { + } catch (NoSuchFileException e) { return UUID.randomUUID().toString(); } } diff --git a/src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java b/src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java index 6828a33c..6e43e0f5 100644 --- a/src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java +++ b/src/test/java/org/cryptomator/cryptofs/DirectoryIdLoaderTest.java @@ -12,29 +12,32 @@ import static org.mockito.Mockito.when; import java.io.IOException; +import java.lang.reflect.Field; import java.nio.ByteBuffer; -import java.nio.channels.SeekableByteChannel; +import java.nio.channels.FileChannel; +import java.nio.channels.spi.AbstractInterruptibleChannel; import java.nio.file.FileSystem; +import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.spi.FileSystemProvider; -import org.cryptomator.cryptofs.mocks.SeekableByteChannelMock; import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; +import org.mockito.Mockito; public class DirectoryIdLoaderTest { @Rule public ExpectedException thrown = ExpectedException.none(); - private FileSystemProvider provider = mock(FileSystemProvider.class); - private FileSystem fileSystem = mock(FileSystem.class); - private Path dirFilePath = mock(Path.class); - private Path otherDirFilePath = mock(Path.class); + private final FileSystemProvider provider = mock(FileSystemProvider.class); + private final FileSystem fileSystem = mock(FileSystem.class); + private final Path dirFilePath = mock(Path.class); + private final Path otherDirFilePath = mock(Path.class); - private DirectoryIdLoader inTest = new DirectoryIdLoader(); + private final DirectoryIdLoader inTest = new DirectoryIdLoader(); @Before public void setup() { @@ -45,8 +48,8 @@ public void setup() { @Test public void testDirectoryIdsForTwoNonExistingFilesDiffer() throws IOException { - doThrow(new IOException()).when(provider).checkAccess(dirFilePath); - doThrow(new IOException()).when(provider).checkAccess(otherDirFilePath); + doThrow(new NoSuchFileException("foo")).when(provider).newFileChannel(eq(dirFilePath), any()); + doThrow(new NoSuchFileException("bar")).when(provider).newFileChannel(eq(otherDirFilePath), any()); String first = inTest.load(dirFilePath); String second = inTest.load(otherDirFilePath); @@ -56,7 +59,7 @@ public void testDirectoryIdsForTwoNonExistingFilesDiffer() throws IOException { @Test public void testDirectoryIdForNonExistingFileIsNotEmpty() throws IOException { - doThrow(new IOException()).when(provider).checkAccess(dirFilePath); + doThrow(new NoSuchFileException("foo")).when(provider).newFileChannel(eq(dirFilePath), any()); String result = inTest.load(dirFilePath); @@ -65,11 +68,17 @@ public void testDirectoryIdForNonExistingFileIsNotEmpty() throws IOException { } @Test - public void testDirectoryIdIsReadFromExistingFile() throws IOException { + public void testDirectoryIdIsReadFromExistingFile() throws IOException, ReflectiveOperationException { String expectedId = "asdüßT°z¬╚‗"; byte[] expectedIdBytes = expectedId.getBytes(UTF_8); - SeekableByteChannel channel = new SeekableByteChannelMock(ByteBuffer.wrap(expectedIdBytes)); - when(provider.newByteChannel(eq(dirFilePath), any())).thenReturn(channel); + FileChannel channel = createFileChannelMock(); + when(provider.newFileChannel(eq(dirFilePath), any())).thenReturn(channel); + when(channel.size()).thenReturn((long) expectedIdBytes.length); + when(channel.read(any(ByteBuffer.class))).then(invocation -> { + ByteBuffer buf = invocation.getArgument(0); + buf.put(expectedIdBytes); + return expectedIdBytes.length; + }); String result = inTest.load(dirFilePath); @@ -77,9 +86,10 @@ public void testDirectoryIdIsReadFromExistingFile() throws IOException { } @Test - public void testIOExceptionWhenExistingFileIsEmpty() throws IOException { - SeekableByteChannel channel = new SeekableByteChannelMock(ByteBuffer.allocate(0)); - when(provider.newByteChannel(eq(dirFilePath), any())).thenReturn(channel); + public void testIOExceptionWhenExistingFileIsEmpty() throws IOException, ReflectiveOperationException { + FileChannel channel = createFileChannelMock(); + when(provider.newFileChannel(eq(dirFilePath), any())).thenReturn(channel); + when(channel.size()).thenReturn(0l); thrown.expect(IOException.class); thrown.expectMessage("Invalid, empty directory file"); @@ -87,4 +97,27 @@ public void testIOExceptionWhenExistingFileIsEmpty() throws IOException { inTest.load(dirFilePath); } + @Test + public void testIOExceptionWhenExistingFileIsTooLarge() throws IOException, ReflectiveOperationException { + FileChannel channel = createFileChannelMock(); + when(provider.newFileChannel(eq(dirFilePath), any())).thenReturn(channel); + when(channel.size()).thenReturn((long) Integer.MAX_VALUE); + + thrown.expect(IOException.class); + thrown.expectMessage("Unexpectedly large directory file"); + + inTest.load(dirFilePath); + } + + private FileChannel createFileChannelMock() throws ReflectiveOperationException { + FileChannel channel = Mockito.mock(FileChannel.class); + Field channelOpenField = AbstractInterruptibleChannel.class.getDeclaredField("open"); + channelOpenField.setAccessible(true); + channelOpenField.set(channel, true); + Field channelCloseLockField = AbstractInterruptibleChannel.class.getDeclaredField("closeLock"); + channelCloseLockField.setAccessible(true); + channelCloseLockField.set(channel, new Object()); + return channel; + } + } From cecae475a205341036b3dba898739d895243b2ac Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 30 May 2017 12:56:08 +0200 Subject: [PATCH 5/6] fixed bug: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ```java Path path = cryptoFs.getPath(“test”); Files.createDirectories(path); // first call succeeded Files.createDirectories(path); // second call fails, dirId file mistakenly deleted during “finally-cleanup” in old code ``` --- .../cryptofs/CryptoFileSystemImpl.java | 22 +++---- .../cryptofs/CryptoFileSystemImplTest.java | 60 ++++++++++--------- 2 files changed, 42 insertions(+), 40 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java b/src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java index 6b8c34c3..a50158c9 100644 --- a/src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java +++ b/src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java @@ -316,19 +316,19 @@ void createDirectory(CryptoPath cleartextDir, FileAttribute... attrs) throws throw new FileAlreadyExistsException(cleartextDir.toString()); } Path ciphertextDirFile = cryptoPathMapper.getCiphertextFilePath(cleartextDir, CiphertextFileType.DIRECTORY); - boolean success = false; + Directory ciphertextDir = cryptoPathMapper.getCiphertextDir(cleartextDir); + // atomically check for FileAlreadyExists and create otherwise: + try (FileChannel channel = FileChannel.open(ciphertextDirFile, EnumSet.of(StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE), attrs)) { + channel.write(ByteBuffer.wrap(ciphertextDir.dirId.getBytes(UTF_8))); + } + // create dir if and only if the dirFile has been created right now (not if it has been created before): try { - Directory ciphertextDir = cryptoPathMapper.getCiphertextDir(cleartextDir); - try (FileChannel channel = FileChannel.open(ciphertextDirFile, EnumSet.of(StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE), attrs)) { - channel.write(ByteBuffer.wrap(ciphertextDir.dirId.getBytes(UTF_8))); - } Files.createDirectories(ciphertextDir.path); - success = true; - } finally { - if (!success) { - Files.delete(ciphertextDirFile); - dirIdProvider.delete(ciphertextDirFile); - } + } catch (IOException e) { + // make sure there is no orphan dir file: + Files.delete(ciphertextDirFile); + dirIdProvider.delete(ciphertextDirFile); + throw e; } } diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java index f67d7cbc..5ca1173c 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java @@ -714,8 +714,8 @@ public void copyDirectoryToAlreadyExistingDir() throws IOException { public class CreateDirectory { - private FileSystemProvider provider = mock(FileSystemProvider.class); - private CryptoFileSystemImpl fileSystem = mock(CryptoFileSystemImpl.class); + private final FileSystemProvider provider = mock(FileSystemProvider.class); + private final CryptoFileSystemImpl fileSystem = mock(CryptoFileSystemImpl.class); @Before public void setup() { @@ -796,37 +796,39 @@ public void createDirectoryCreatesDirectoryIfConditonsAreMet() throws IOExceptio } @Test - public void createDirectoryClearsDirIdAndDeletesDirFileIfWritingDirFileFails() throws IOException { + public void createDirectoryClearsDirIdAndDeletesDirFileIfCreatingDirFails() throws IOException { CryptoPath path = mock(CryptoPath.class); CryptoPath parent = mock(CryptoPath.class); - Path cyphertextParent = mock(Path.class); - Path cyphertextFile = mock(Path.class); - Path cyphertextDirFile = mock(Path.class); - Path cyphertextDirPath = mock(Path.class); + Path ciphertextParent = mock(Path.class, "ciphertextParent"); + Path ciphertextFile = mock(Path.class, "ciphertextFile"); + Path ciphertextDirFile = mock(Path.class, "ciphertextDirFile"); + Path ciphertextDirPath = mock(Path.class, "ciphertextDir"); String dirId = "DirId1234ABC"; FileChannelMock channel = new FileChannelMock(100); - Directory cyphertextDir = new Directory(dirId, cyphertextDirPath); + Directory ciphertextDir = new Directory(dirId, ciphertextDirPath); when(path.getParent()).thenReturn(parent); - when(cryptoPathMapper.getCiphertextDirPath(parent)).thenReturn(cyphertextParent); - when(cryptoPathMapper.getCiphertextFilePath(path, CiphertextFileType.FILE)).thenReturn(cyphertextFile); - when(cryptoPathMapper.getCiphertextFilePath(path, CiphertextFileType.DIRECTORY)).thenReturn(cyphertextDirFile); - when(cryptoPathMapper.getCiphertextDir(path)).thenReturn(cyphertextDir); - when(cyphertextParent.getFileSystem()).thenReturn(fileSystem); - when(cyphertextFile.getFileSystem()).thenReturn(fileSystem); - when(cyphertextDirFile.getFileSystem()).thenReturn(fileSystem); - when(cyphertextDirPath.getFileSystem()).thenReturn(fileSystem); - when(provider.newFileChannel(cyphertextDirFile, EnumSet.of(StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE))).thenReturn(channel); - IOException expectedException = new IOException(); - channel.setFailOnNextOperation(expectedException); - doThrow(NoSuchFileException.class).when(provider).checkAccess(cyphertextFile); + when(cryptoPathMapper.getCiphertextDirPath(parent)).thenReturn(ciphertextParent); + when(cryptoPathMapper.getCiphertextFilePath(path, CiphertextFileType.FILE)).thenReturn(ciphertextFile); + when(cryptoPathMapper.getCiphertextFilePath(path, CiphertextFileType.DIRECTORY)).thenReturn(ciphertextDirFile); + when(cryptoPathMapper.getCiphertextDir(path)).thenReturn(ciphertextDir); + when(ciphertextParent.getFileSystem()).thenReturn(fileSystem); + when(ciphertextFile.getFileSystem()).thenReturn(fileSystem); + when(ciphertextDirFile.getFileSystem()).thenReturn(fileSystem); + when(ciphertextDirPath.getFileSystem()).thenReturn(fileSystem); + doThrow(new NoSuchFileException("ciphertextFile")).when(provider).checkAccess(ciphertextFile); + when(provider.newFileChannel(ciphertextDirFile, EnumSet.of(StandardOpenOption.CREATE_NEW, StandardOpenOption.WRITE))).thenReturn(channel); - thrown.expect(is(expectedException)); + // make createDirectory with an FileSystemException during Files.createDirectories(ciphertextDirPath) + doThrow(new IOException()).when(provider).createDirectory(ciphertextDirPath); + when(ciphertextDirPath.toAbsolutePath()).thenReturn(ciphertextDirPath); + when(ciphertextDirPath.getParent()).thenReturn(null); + thrown.expect(IOException.class); try { inTest.createDirectory(path); } finally { - verify(provider).delete(cyphertextDirFile); - verify(dirIdProvider).delete(cyphertextDirFile); + verify(provider).delete(ciphertextDirFile); + verify(dirIdProvider).delete(ciphertextDirFile); } } @@ -834,8 +836,8 @@ public void createDirectoryClearsDirIdAndDeletesDirFileIfWritingDirFileFails() t public class IsHidden { - private FileSystemProvider provider = mock(FileSystemProvider.class); - private CryptoFileSystemImpl fileSystem = mock(CryptoFileSystemImpl.class); + private final FileSystemProvider provider = mock(FileSystemProvider.class); + private final CryptoFileSystemImpl fileSystem = mock(CryptoFileSystemImpl.class); @Before public void setup() { @@ -886,8 +888,8 @@ public void isHiddenReturnsFalseIfDosFileAttributeViewIsAvailableAndIsHiddenIsFa public class CheckAccess { - private FileSystemProvider provider = mock(FileSystemProvider.class); - private CryptoFileSystemImpl fileSystem = mock(CryptoFileSystemImpl.class); + private final FileSystemProvider provider = mock(FileSystemProvider.class); + private final CryptoFileSystemImpl fileSystem = mock(CryptoFileSystemImpl.class); @Before public void setup() { @@ -1110,8 +1112,8 @@ public void testAccessModeContainsOnlyKnownValues() { public class SetAttribute { - private FileSystemProvider provider = mock(FileSystemProvider.class); - private CryptoFileSystemImpl fileSystem = mock(CryptoFileSystemImpl.class); + private final FileSystemProvider provider = mock(FileSystemProvider.class); + private final CryptoFileSystemImpl fileSystem = mock(CryptoFileSystemImpl.class); @Before public void setup() { From f9021beb05ad2aa8cf5b53f8287d79095ab8d98b Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 30 May 2017 13:10:56 +0200 Subject: [PATCH 6/6] Updated dependencies, preparing 1.2.3 --- pom.xml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pom.xml b/pom.xml index 5271efaa..a64cb03d 100644 --- a/pom.xml +++ b/pom.xml @@ -2,7 +2,7 @@ 4.0.0 org.cryptomator cryptofs - 1.3.0-SNAPSHOT + 1.2.3 Cryptomator Crypto Filesystem This library provides the Java filesystem provider used by Cryptomator. https://github.com/cryptomator/cryptofs @@ -16,7 +16,7 @@ 1.8 1.1.1 - 2.10 + 2.11 21.0 3.5 1.7.25 @@ -108,7 +108,7 @@ org.mockito mockito-core - 2.7.21 + 2.8.9 test @@ -132,7 +132,7 @@ org.bouncycastle bcprov-jdk15on - 1.56 + 1.57 test @@ -191,7 +191,7 @@ com.codacy codacy-coverage-reporter - 1.0.13 + 2.0.0 assembly @@ -282,7 +282,7 @@ maven-dependency-plugin - 3.0.0 + 3.0.1 generate-dependency-list