From 694f6e0eda03feb71b0e751bddb76b123cde939a Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Fri, 14 Apr 2023 11:16:00 +0200 Subject: [PATCH 1/3] Fixes #170 --- .../cryptofs/CryptoFileSystemImpl.java | 7 ++++++- .../cryptomator/cryptofs/fh/OpenCryptoFile.java | 16 +++++++++++++--- .../cryptomator/cryptofs/fh/OpenCryptoFiles.java | 16 +++++++++++++++- ...ryptoFileChannelWriteReadIntegrationTest.java | 16 ++++++++++++++++ .../cryptofs/CryptoFileSystemImplTest.java | 5 +++++ 5 files changed, 55 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java b/src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java index 887bb189..4f8397ce 100644 --- a/src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java +++ b/src/main/java/org/cryptomator/cryptofs/CryptoFileSystemImpl.java @@ -421,10 +421,15 @@ void delete(CryptoPath cleartextPath) throws IOException { CiphertextFilePath ciphertextPath = cryptoPathMapper.getCiphertextFilePath(cleartextPath); switch (ciphertextFileType) { case DIRECTORY -> deleteDirectory(cleartextPath, ciphertextPath); - case FILE, SYMLINK -> Files.walkFileTree(ciphertextPath.getRawPath(), DeletingFileVisitor.INSTANCE); + case FILE, SYMLINK -> deleteFileOrSymlink(ciphertextPath); } } + private void deleteFileOrSymlink(CiphertextFilePath ciphertextPath) throws IOException { + openCryptoFiles.delete(ciphertextPath.getFilePath()); + Files.walkFileTree(ciphertextPath.getRawPath(), DeletingFileVisitor.INSTANCE); + } + private void deleteDirectory(CryptoPath cleartextPath, CiphertextFilePath ciphertextPath) throws IOException { Path ciphertextDir = cryptoPathMapper.getCiphertextDir(cleartextPath).path; Path ciphertextDirFile = ciphertextPath.getDirFilePath(); diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java index 71f1afb5..a07b5db2 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java @@ -66,6 +66,9 @@ public OpenCryptoFile(FileCloseListener listener, ChunkCache chunkCache, Cryptor */ public synchronized FileChannel newFileChannel(EffectiveOpenOptions options, FileAttribute... attrs) throws IOException { Path path = currentFilePath.get(); + if (path == null) { + throw new IllegalStateException("Cannot create file channel to deleted file"); + } FileChannel ciphertextFileChannel = null; CleartextFileChannel cleartextFileChannel = null; try { @@ -172,8 +175,12 @@ public Path getCurrentFilePath() { return currentFilePath.get(); } - public void setCurrentFilePath(Path currentFilePath) { - this.currentFilePath.set(currentFilePath); + /** + * Updates the current ciphertext file path, if it is not already set to null (i.e., the openCryptoFile is deleted) + * @param newFilePath new ciphertext path + */ + public void updateCurrentFilePath(Path newFilePath) { + currentFilePath.updateAndGet(p -> p == null ? null : newFilePath); } private synchronized void channelClosed(CleartextFileChannel cleartextFileChannel) throws IOException { @@ -192,7 +199,10 @@ private synchronized void channelClosed(CleartextFileChannel cleartextFileChanne @Override public void close() { - listener.close(currentFilePath.get(), this); + var p = currentFilePath.get(); + if(p != null) { + listener.close(p, this); + } } @Override diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java index 4186e248..4e872250 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java @@ -81,6 +81,20 @@ public ByteBuffer readCiphertextFile(Path ciphertextPath, EffectiveOpenOptions o } } + /** + * Removes a ciphertextPath to {@link OpenCryptoFile} mapping, if it exists, and sets the path of the openCryptoFile to null. + * + * @param ciphertextPath The ciphertext file path to invalidate + */ + public void delete(Path ciphertextPath) { + openCryptoFiles.compute(ciphertextPath, (p, openFile) -> { + if (openFile != null) { + openFile.updateCurrentFilePath(null); + } + return null; + }); + } + /** * Prepares to update any open file references during a move operation. * MUST be invoked using a try-with-resource statement and committed after the physical file move succeeded. @@ -137,7 +151,7 @@ public void commit() { throw new IllegalStateException(); } if (openCryptoFile != null) { - openCryptoFile.setCurrentFilePath(dst); + openCryptoFile.updateCurrentFilePath(dst); } openCryptoFiles.remove(src, openCryptoFile); committed = true; diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java index 161a908f..5e34c9f6 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java @@ -48,6 +48,7 @@ import java.util.Arrays; import java.util.List; import java.util.Random; +import java.util.Set; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.stream.IntStream; @@ -556,6 +557,21 @@ public void testClosingChannelOfDeletedFileDoesNotThrow() { }); Assertions.assertTrue(Files.notExists(file)); } + + //https://github.com/cryptomator/cryptofs/issues/170 + @Test + public void testWriteThenDeleteThenRead() throws IOException { + var bufToWrite = ByteBuffer.wrap("delete me".getBytes(StandardCharsets.UTF_8)); + final int bytesRead; + try (var ch = FileChannel.open(file, CREATE_NEW, WRITE)) { + ch.write(bufToWrite); + Files.delete(file); + try (var ch2 = fileSystem.provider().newFileChannel(file, Set.of(CREATE, READ, WRITE))) { + bytesRead = ch2.read(ByteBuffer.allocate(bufToWrite.capacity())); + } + } + Assertions.assertEquals(-1, bytesRead); + } } } diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java index a133999f..5fc8aa32 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoFileSystemImplTest.java @@ -72,6 +72,7 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -558,6 +559,7 @@ public class Delete { private final CryptoPath cleartextPath = mock(CryptoPath.class, "cleartext"); private final Path ciphertextRawPath = mock(Path.class, "d/00/00/path.c9r"); private final Path ciphertextDirFilePath = mock(Path.class, "d/00/00/path.c9r/dir.c9r"); + private final Path ciphertextFilePath = mock(Path.class, "d/00/00/path.c9r"); private final Path ciphertextDirPath = mock(Path.class, "d/FF/FF/"); private final CiphertextFilePath ciphertextPath = mock(CiphertextFilePath.class, "ciphertext"); private final FileSystem physicalFs = mock(FileSystem.class); @@ -574,6 +576,7 @@ public void setup() throws IOException { when(ciphertextRawPath.resolve("dir.c9r")).thenReturn(ciphertextDirFilePath); when(cryptoPathMapper.getCiphertextFilePath(cleartextPath)).thenReturn(ciphertextPath); when(ciphertextPath.getRawPath()).thenReturn(ciphertextRawPath); + when(ciphertextPath.getFilePath()).thenReturn(ciphertextFilePath); when(ciphertextPath.getDirFilePath()).thenReturn(ciphertextDirFilePath); when(cryptoPathMapper.getCiphertextDir(cleartextPath)).thenReturn(new CiphertextDirectory("foo", ciphertextDirPath)); when(physicalFsProv.readAttributes(ciphertextRawPath, BasicFileAttributes.class, LinkOption.NOFOLLOW_LINKS)).thenReturn(ciphertextPathAttr); @@ -590,10 +593,12 @@ public void testDeleteRootFails() { public void testDeleteExistingFile() throws IOException { when(cryptoPathMapper.getCiphertextFileType(cleartextPath)).thenReturn(CiphertextFileType.FILE); when(physicalFsProv.deleteIfExists(ciphertextRawPath)).thenReturn(true); + doNothing().when(openCryptoFiles).delete(Mockito.any()); inTest.delete(cleartextPath); verify(readonlyFlag).assertWritable(); + verify(openCryptoFiles).delete(ciphertextFilePath); verify(physicalFsProv).deleteIfExists(ciphertextRawPath); } From f9f3d007d69f41d751ea7f6fb4cd64e5bc93e803 Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Fri, 14 Apr 2023 11:52:49 +0200 Subject: [PATCH 2/3] rework persistLastModified() related to #169 --- .../cryptofs/ch/CleartextFileChannel.java | 22 ++++++++++++++----- .../cryptofs/fh/OpenCryptoFileModule.java | 11 +--------- .../cryptofs/ch/CleartextFileChannelTest.java | 18 ++++++++++----- 3 files changed, 30 insertions(+), 21 deletions(-) diff --git a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java index c3400f81..97791365 100644 --- a/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java +++ b/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java @@ -6,6 +6,7 @@ import org.cryptomator.cryptofs.fh.BufferPool; import org.cryptomator.cryptofs.fh.Chunk; import org.cryptomator.cryptofs.fh.ChunkCache; +import org.cryptomator.cryptofs.fh.CurrentOpenFilePath; import org.cryptomator.cryptofs.fh.ExceptionsDuringWrite; import org.cryptomator.cryptofs.fh.FileHeaderHolder; import org.cryptomator.cryptofs.fh.OpenFileModifiedDate; @@ -22,13 +23,14 @@ import java.nio.channels.FileLock; import java.nio.channels.NonReadableChannelException; import java.nio.channels.NonWritableChannelException; +import java.nio.file.NoSuchFileException; +import java.nio.file.Path; import java.nio.file.attribute.BasicFileAttributeView; import java.nio.file.attribute.FileTime; import java.time.Instant; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.locks.ReadWriteLock; -import java.util.function.Supplier; import static java.lang.Math.max; import static java.lang.Math.min; @@ -44,15 +46,15 @@ public class CleartextFileChannel extends AbstractFileChannel { private final ChunkCache chunkCache; private final BufferPool bufferPool; private final EffectiveOpenOptions options; + private final AtomicReference currentFilePath; private final AtomicLong fileSize; private final AtomicReference lastModified; - private final Supplier attrViewProvider; private final ExceptionsDuringWrite exceptionsDuringWrite; private final ChannelCloseListener closeListener; private final CryptoFileSystemStats stats; @Inject - public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeaderHolder fileHeaderHolder, ReadWriteLock readWriteLock, Cryptor cryptor, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference lastModified, Supplier attrViewProvider, ExceptionsDuringWrite exceptionsDuringWrite, ChannelCloseListener closeListener, CryptoFileSystemStats stats) { + public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeaderHolder fileHeaderHolder, ReadWriteLock readWriteLock, Cryptor cryptor, ChunkCache chunkCache, BufferPool bufferPool, EffectiveOpenOptions options, @OpenFileSize AtomicLong fileSize, @OpenFileModifiedDate AtomicReference lastModified, @CurrentOpenFilePath AtomicReference currentPath, ExceptionsDuringWrite exceptionsDuringWrite, ChannelCloseListener closeListener, CryptoFileSystemStats stats) { super(readWriteLock); this.ciphertextFileChannel = ciphertextFileChannel; this.fileHeaderHolder = fileHeaderHolder; @@ -60,9 +62,9 @@ public CleartextFileChannel(FileChannel ciphertextFileChannel, FileHeaderHolder this.chunkCache = chunkCache; this.bufferPool = bufferPool; this.options = options; + this.currentFilePath = currentPath; this.fileSize = fileSize; this.lastModified = lastModified; - this.attrViewProvider = attrViewProvider; this.exceptionsDuringWrite = exceptionsDuringWrite; this.closeListener = closeListener; this.stats = stats; @@ -246,7 +248,13 @@ private void flush() throws IOException { private void persistLastModified() throws IOException { FileTime lastModifiedTime = isWritable() ? FileTime.from(lastModified.get()) : null; FileTime lastAccessTime = FileTime.from(Instant.now()); - attrViewProvider.get().setTimes(lastModifiedTime, lastAccessTime, null); + var p = currentFilePath.get(); + if (p != null) { + p.getFileSystem().provider()// + .getFileAttributeView(p, BasicFileAttributeView.class) + .setTimes(lastModifiedTime, lastAccessTime, null); + } + } @Override @@ -316,8 +324,10 @@ protected void implCloseChannel() throws IOException { flush(); try { persistLastModified(); - } catch (IOException e) { + } catch (NoSuchFileException nsfe) { //no-op, see https://github.com/cryptomator/cryptofs/issues/169 + } catch (IOException e) { + //only best effort attempt LOG.warn("Failed to persist last modified timestamp for encrypted file: {}", e.getMessage()); } } finally { diff --git a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileModule.java b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileModule.java index 83d587ee..aceb7484 100644 --- a/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileModule.java +++ b/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFileModule.java @@ -28,20 +28,11 @@ public ReadWriteLock provideReadWriteLock() { @Provides @OpenFileScoped - @CurrentOpenFilePath // TODO: do we still need this? only used in logging. + @CurrentOpenFilePath public AtomicReference provideCurrentPath(@OriginalOpenFilePath Path originalPath) { return new AtomicReference<>(originalPath); } - @Provides - @OpenFileScoped - public Supplier provideBasicFileAttributeViewSupplier(@CurrentOpenFilePath AtomicReference currentPath) { - return () -> { - Path path = currentPath.get(); - return path.getFileSystem().provider().getFileAttributeView(path, BasicFileAttributeView.class); - }; - } - @Provides @OpenFileScoped @OpenFileModifiedDate diff --git a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java index 9009ae48..8b07565a 100644 --- a/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java +++ b/src/test/java/org/cryptomator/cryptofs/ch/CleartextFileChannelTest.java @@ -32,8 +32,11 @@ import java.nio.channels.NonWritableChannelException; import java.nio.channels.ReadableByteChannel; import java.nio.channels.WritableByteChannel; +import java.nio.file.FileSystem; +import java.nio.file.Path; import java.nio.file.attribute.BasicFileAttributeView; import java.nio.file.attribute.FileTime; +import java.nio.file.spi.FileSystemProvider; import java.time.Instant; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicLong; @@ -66,9 +69,10 @@ public class CleartextFileChannelTest { private FileHeaderHolder headerHolder = mock(FileHeaderHolder.class); private AtomicBoolean headerIsPersisted = mock(AtomicBoolean.class); private EffectiveOpenOptions options = mock(EffectiveOpenOptions.class); + private Path filePath = Mockito.mock(Path.class,"/foo/bar"); + private AtomicReference currentFilePath = new AtomicReference<>(filePath); private AtomicLong fileSize = new AtomicLong(100); private AtomicReference lastModified = new AtomicReference<>(Instant.ofEpochMilli(0)); - private Supplier attributeViewSupplier = mock(Supplier.class); private BasicFileAttributeView attributeView = mock(BasicFileAttributeView.class); private ExceptionsDuringWrite exceptionsDuringWrite = mock(ExceptionsDuringWrite.class); private ChannelCloseListener closeListener = mock(ChannelCloseListener.class); @@ -88,11 +92,15 @@ public void setUp() throws IOException { when(headerIsPersisted.getAndSet(anyBoolean())).thenReturn(true); when(fileContentCryptor.cleartextChunkSize()).thenReturn(100); when(fileContentCryptor.ciphertextChunkSize()).thenReturn(110); - when(attributeViewSupplier.get()).thenReturn(attributeView); + var fs = Mockito.mock(FileSystem.class); + var fsProvider = Mockito.mock(FileSystemProvider.class); + when(filePath.getFileSystem()).thenReturn(fs); + when(fs.provider()).thenReturn(fsProvider); + when(fsProvider.getFileAttributeView(filePath,BasicFileAttributeView.class)).thenReturn(attributeView); when(readWriteLock.readLock()).thenReturn(readLock); when(readWriteLock.writeLock()).thenReturn(writeLock); - inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, attributeViewSupplier, exceptionsDuringWrite, closeListener, stats); + inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats); } @Test @@ -345,7 +353,7 @@ public void testReadFromMultipleChunks() throws IOException { fileSize.set(5_000_000_100l); // initial cleartext size will be 5_000_000_100l when(options.readable()).thenReturn(true); - inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, attributeViewSupplier, exceptionsDuringWrite, closeListener, stats); + inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats); ByteBuffer buf = ByteBuffer.allocate(10); // A read from frist chunk: @@ -517,7 +525,7 @@ public void testWriteHeaderFailsResetsPersistenceState() throws IOException { public void testDontRewriteHeader() throws IOException { when(options.writable()).thenReturn(true); when(headerIsPersisted.get()).thenReturn(true); - inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, attributeViewSupplier, exceptionsDuringWrite, closeListener, stats); + inTest = new CleartextFileChannel(ciphertextFileChannel, headerHolder, readWriteLock, cryptor, chunkCache, bufferPool, options, fileSize, lastModified, currentFilePath, exceptionsDuringWrite, closeListener, stats); inTest.force(true); From 2a22af17abc35d271992836146964f9939c25d9b Mon Sep 17 00:00:00 2001 From: Armin Schrenk Date: Fri, 14 Apr 2023 16:42:40 +0200 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Sebastian Stenzel --- .../cryptofs/CryptoFileChannelWriteReadIntegrationTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java index 5e34c9f6..aa725db0 100644 --- a/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java +++ b/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java @@ -561,7 +561,7 @@ public void testClosingChannelOfDeletedFileDoesNotThrow() { //https://github.com/cryptomator/cryptofs/issues/170 @Test public void testWriteThenDeleteThenRead() throws IOException { - var bufToWrite = ByteBuffer.wrap("delete me".getBytes(StandardCharsets.UTF_8)); + var bufToWrite = StandardCharsets.UTF_8.encode("delete me"); final int bytesRead; try (var ch = FileChannel.open(file, CREATE_NEW, WRITE)) { ch.write(bufToWrite);