Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Apr 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
16 changes: 13 additions & 3 deletions src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 15 additions & 1 deletion src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFiles.java
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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);

Expand Down