Merge pull request #172 from cryptomator/feature/170-nullify-path-on-…

…delete

Feature: Remove deleted file from openFiles map and set file path to null

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();

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,25 +46,25 @@ public class CleartextFileChannel extends AbstractFileChannel {
 	private final ChunkCache chunkCache;
 	private final BufferPool bufferPool;
 	private final EffectiveOpenOptions options;
+	private final AtomicReference<Path> currentFilePath;
 	private final AtomicLong fileSize;
 	private final AtomicReference<Instant> lastModified;
-	private final Supplier<BasicFileAttributeView> 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<Instant> lastModified, Supplier<BasicFileAttributeView> 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<Instant> lastModified, @CurrentOpenFilePath AtomicReference<Path> currentPath, ExceptionsDuringWrite exceptionsDuringWrite, ChannelCloseListener closeListener, CryptoFileSystemStats stats) {
 		super(readWriteLock);
 		this.ciphertextFileChannel = ciphertextFileChannel;
 		this.fileHeaderHolder = fileHeaderHolder;
 		this.cryptor = cryptor;
 		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 {

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

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<Path> provideCurrentPath(@OriginalOpenFilePath Path originalPath) {
 		return new AtomicReference<>(originalPath);
 	}
 
-	@Provides
-	@OpenFileScoped
-	public Supplier<BasicFileAttributeView> provideBasicFileAttributeViewSupplier(@CurrentOpenFilePath AtomicReference<Path> currentPath) {
-		return () -> {
-			Path path = currentPath.get();
-			return path.getFileSystem().provider().getFileAttributeView(path, BasicFileAttributeView.class);
-		};
-	}
-
 	@Provides
 	@OpenFileScoped
 	@OpenFileModifiedDate

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;

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 = StandardCharsets.UTF_8.encode("delete me");
+			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);
+		}
 	}
 
 }

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);
 		}
 

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<Path> currentFilePath = new AtomicReference<>(filePath);
 	private AtomicLong fileSize = new AtomicLong(100);
 	private AtomicReference<Instant> lastModified = new AtomicReference<>(Instant.ofEpochMilli(0));
-	private Supplier<BasicFileAttributeView> 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);