From cecae475a205341036b3dba898739d895243b2ac Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Tue, 30 May 2017 12:56:08 +0200 Subject: [PATCH] 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() {