Bug 2721 - Improve SFTP server to make remove always work on ZFS/Btrfs
Summary: Improve SFTP server to make remove always work on ZFS/Btrfs
Status: CLOSED WONTFIX
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: sftp-server (show other bugs)
Version: 7.5p1
Hardware: Other All
: P5 normal
Assignee: Assigned to nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-05-23 16:51 AEST by Stefan Walter
Modified: 2018-04-06 12:26 AEST (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Stefan Walter 2017-05-23 16:51:09 AEST
The code in process_remove() in sftp-server.c just tries a simple
unlink for the file to be deleted. This does not work well on ZFS or Btrfs if the filesystem/pool is completely full, which is a well known problem of CoW filesystems. See here ofr instance:

  http://www.surlyjake.com/blog/2010/01/19/zfs-cant-rm-no-space-left-on-device/

Trying to delete a file on a full filesystem just fails:

sftp> rm some_file
Removing /full/some_file
Couldn't delete file: Failure

The trick is to first do a truncate() on a file containing data to free up data blocks. In this case, truncating the file to be deleted before the unlink() would solve the problem.
Comment 1 Darren Tucker 2017-05-24 09:59:38 AEST
Err, that sounds like a data corruption bug in the case where you have two hard links to a file and delete one.
Comment 2 Stefan Walter 2017-05-24 16:10:00 AEST
Checking for symbolic link and hard link count should be part of the process. The logic should be something like this:

if unlink() fails with EDQUOT or ENOSPC
  stat file
  if not symbolic link and hard link count is 1
    open for r/w (implicit truncate) and close
    unlink again

or, more complex and safe:

if unlink() fails with EDQUOT or ENOSPC
  stat file
  if not symbolic link and hard link count is 1
    open for append
    obtain exclusive lock on file handle
    stat file again
    if still the same file (inode, owner, mode, etc. are the same)
      truncate
      unlink again
    unlock and close

The question of course is, can a malicious user on the system exploit any race conditions in this process.
Comment 3 Damien Miller 2017-05-25 10:31:43 AEST
(In reply to Stefan Walter from comment #2)

> The question of course is, can a malicious user on the system
> exploit any race conditions in this process.

sure - it's racy.

I don't think sftp should attempt to do magic to work around filesystem corner cases, especially when rm(1) doesn't.

OTOH we should offer users the ability to truncate a file themselves if we don't already.

Does "put /dev/null file-to-delete" work to truncate? AFAIK it should because we open(.., O_TRUNC, ..)
Comment 4 Stefan Walter 2017-05-26 17:04:12 AEST
Using put of a zero size file has been the known workaround for a while. It is not optimal in situations where SFTP is used by an application and not directly by a user. We ran into the issue with a backup software that used sftp as backend.

I had to post a change request with that product to implement this workaround. As you can imagine, they will argue that they should not care what kind of FS is used on the remote server and that it is up to the SFTP server software to deal with this. Both of you are right but that doesn't make the problem go away. :)

Anyway, you can close this as WONTFIX, I will pursue the other change request.
Comment 5 Damien Miller 2017-06-09 13:44:01 AEST
sorry, this isn't something I think we should implement in sftp-server
Comment 6 Damien Miller 2018-04-06 12:26:47 AEST
Close all resolved bugs after release of OpenSSH 7.7.