Truncating a channel which is currently written can create invalid ciphertext

[infeo]

Summary

Library version: 2.7.1

Truncating a channel which is currently written to can create invalid ciphertext/unreadable file.

Description

In our test suite exists a test, which truncates a file while another thread writes data to the same file:

cryptofs/src/test/java/org/cryptomator/cryptofs/CryptoFileChannelWriteReadIntegrationTest.java

Lines 620 to 647 in 4d6a56b

	public void testConcurrentWriteAndTruncate() throws IOException {
	AtomicBoolean keepWriting = new AtomicBoolean(true);
	ByteBuffer buf = ByteBuffer.wrap("the quick brown fox jumps over the lazy dog".getBytes(StandardCharsets.UTF_8));
	var executor = Executors.newCachedThreadPool();
	try (FileChannel writingChannel = FileChannel.open(file, WRITE, CREATE)) {
	executor.submit(() -> {
	while (keepWriting.get()) {
	try {
	writingChannel.write(buf);
	} catch (IOException e) {
	throw new UncheckedIOException(e);
	}
	buf.flip();
	}
	});
	try (FileChannel truncatingChannel = FileChannel.open(file, WRITE, TRUNCATE_EXISTING)) {
	keepWriting.set(false);
	}
	executor.shutdown();
	}
	Assertions.assertDoesNotThrow(() -> {
	try (FileChannel readingChannel = FileChannel.open(file, READ)) {
	var dst = ByteBuffer.allocate(buf.capacity());
	readingChannel.read(dst);
	}
	});
	}

This test fails in very rare cases, e.g. in CI run 12694328432. If the logs are not available anymore, you can download them here

You can trigger the failure with higher probability, if you add a new line after line 634 with Thread.sleep(2000);.

[infeo]

I'm pretty sure we have a synchronization issue here. When opening the truncating channel, we directly work on the ciphertextchannel and only evict stale chunks:

cryptofs/src/main/java/org/cryptomator/cryptofs/fh/OpenCryptoFile.java

Lines 77 to 81 in 4d6a56b

	if (options.truncateExisting()) {
	chunkCache.invalidateStale();
	ciphertextFileChannel.truncate(cryptor.fileHeaderCryptor().headerSize());
	fileSize.set(0);
	}

Looking into the truncate method of the cleartext channel, there are way more things done:

cryptofs/src/main/java/org/cryptomator/cryptofs/ch/CleartextFileChannel.java

Lines 197 to 217 in 4d6a56b

	protected void truncateLocked(long newSize) throws IOException {
	Preconditions.checkArgument(newSize >= 0);
	if (newSize < fileSize.get()) {
	int cleartextChunkSize = cryptor.fileContentCryptor().cleartextChunkSize();
	long indexOfLastChunk = (newSize + cleartextChunkSize - 1) / cleartextChunkSize - 1;
	int sizeOfIncompleteChunk = (int) (newSize % cleartextChunkSize); // known to fit in int, because cleartextChunkSize is int
	if (sizeOfIncompleteChunk > 0) {
	try (var chunk = chunkCache.getChunk(indexOfLastChunk)) {
	chunk.data().limit(sizeOfIncompleteChunk);
	chunk.dirty().set(true);
	}
	}
	long ciphertextFileSize = cryptor.fileHeaderCryptor().headerSize() + cryptor.fileContentCryptor().ciphertextSize(newSize);
	chunkCache.flush();
	chunkCache.invalidateStale(); // make sure no chunks _after_ newSize exist that would otherwise be written during the next cache eviction
	ciphertextFileChannel.truncate(ciphertextFileSize);
	position = min(newSize, position);
	fileSize.set(newSize);
	lastModified.set(Instant.now());
	}
	}