Ciphertext filechannels close() might change lastModified timestamp

[infeo]

When opening a filechannel to a not previously opened file in cryptofs, an OpenCryptoFile for this file (path) is instantiated and OpenCryptoFile::newFileChannel-method is called on it.

There the ciphertext filechannel (which is the one actually writing to the underlying filesystem) is created and linked to the returned cleartext file channel. When closing the cleartext file channel via channel.close(), the "close chain" runs over JDKs AbstractInteruptibleChannel.close() and the implementations of the implCloseChannel() methods in AbstractFileChannel and last and most importantly, the CleartextFileChannel:

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

Lines 322 to 337 in 6f04f2a

	protected void implCloseChannel() throws IOException {
	try {
	flush();
	try {
	persistLastModified();
	} 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 {
	super.implCloseChannel();
	closeListener.closed(this);
	}
	}

• The flush()-method writes, if necessary the file header and otherwise empties the chunk cache.
• Afterwards, the lastModified timestamp is corrected (might be wrong on storage due to caching).
• Finally, the super impl of implCloseChannel (does not perform any IO)
• and a close listener is called. The latter is a method in OpenCryptoFile, set via the Dagger ChannelComponent:
  

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

  Lines 186 to 198 in 6f04f2a

  	private synchronized void channelClosed(CleartextFileChannel cleartextFileChannel) throws IOException {
  	try {
  	FileChannel ciphertextFileChannel = openChannels.remove(cleartextFileChannel);
  	if (ciphertextFileChannel != null) {
  	chunkIO.unregisterChannel(ciphertextFileChannel);
  	ciphertextFileChannel.close();
  	}
  	} finally {
  	if (openChannels.isEmpty()) {
  	close();
  	}
  	}
  	}

It basically deregisters the channel from some pools/queues and closes the ciphertext file channel.

Even though we explicitly write the last modifed time to storage, it might be changed afterwards because we never flush the ciphertextchannel explicitly before setting the last modified time. Hence, if there are unwritten changes in the channel, it is only written on close, which is after correcting the last modified time, effectively nullifying the correction.

The flush call only regards the chunk cache, which might use other (ciphertext) file channels.

[infeo]

For a better understanding, what happens during close, I created a (simplified) sequence diagram:

[infeo]

I discussed this with @overheadhunter today. Since changes to cryptofs have a higher bug/regression risk, we decided to go for the minimal change to resolve the issue: Flush the channel before persisting lastModified. There will be still a small gap between persistLastModified() and ciphertextChannel.close(), but with this fix it is always ensured, that the last closing channel persists the lastModified date correctly. And the lib consumer recieves the in-filesystem lastModified object, e.g. only reads from the encrypted file, if no file channel is open.