Created attachment 806898 [details]
patch adding mch_copy_sec() to vim_rename()

Description of problem:
When editing files on folders mounted via sshfs, and you have backupdir set to a directory outside of that mount point, vim nags you with "Press ENTER or type command to continue" after the file has been written and expects you to pess a key. This is very annoying.

Trying to find out what causes this, I ran strace on vim. This is the output when I enter ":w":

select(1, [0], NULL, [0], NULL)         = 1 (in [0])
read(0, ":", 4096)                      = 1
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
write(1, "\33[?25l\33[63;104H\33[K\33[63;1H:", 26) = 26
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
ioctl(0, SNDCTL_TMR_START or SNDRV_TIMER_IOCTL_TREAD or TCSETS, {B38400 opost -isig -icanon -echo ...}) = 0
ioctl(0, SNDCTL_TMR_TIMEBASE or SNDRV_TIMER_IOCTL_NEXT_DEVICE or TCGETS, {B38400 opost -isig -icanon -echo ...}) = 0
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
write(1, "\33[?12l\33[?25h", 12)        = 12
select(1, [0], NULL, [0], {4, 0})       = 1 (in [0], left {3, 822227})
select(1, [0], NULL, [0], NULL)         = 1 (in [0])
read(0, "w", 4096)                      = 1
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
write(1, "w", 1)                        = 1
select(1, [0], NULL, [0], {4, 0})       = 1 (in [0], left {3, 913742})
select(1, [0], NULL, [0], NULL)         = 1 (in [0])
read(0, "\r", 4096)                     = 1
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
write(1, "\r", 1)                       = 1
stat("/home/rtc/foo/foofoobar", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
access("/home/rtc/foo/foofoobar", W_OK) = 0
open(".", O_RDONLY)                     = 4
fchdir(4)                               = 0
chdir("foo")                            = 0
getcwd("/home/rtc/foo", 4096)           = 14
fchdir(4)                               = 0
close(4)                                = 0
write(1, "\33[?25l\"foo/foofoobar\"", 21) = 21
stat("foo/foofoobar", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
access("foo/foofoobar", W_OK)           = 0
getxattr("foo/foofoobar", "system.posix_acl_access", 0x7fff095acc20, 132) = -1 EOPNOTSUPP (Operation not supported)
lstat("foo/foofoobar", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
lstat("foo/4913", 0x7fff095acf20)       = -1 ENOENT (No such file or directory)
open("foo/4913", O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW, 0100664) = 4
fchown(4, 1000, 1000)                   = 0
stat("foo/4913", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
close(4)                                = 0
unlink("foo/4913")                      = 0
stat("/home/rtc/bar/foofoobar~", 0x7fff095acd00) = -1 ENOENT (No such file or directory)
stat("foo/foofoobar", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
stat("/home/rtc/bar/foofoobar~", 0x7fff095abcc0) = -1 ENOENT (No such file or directory)
unlink("/home/rtc/bar/foofoobar~")      = -1 ENOENT (No such file or directory)
rename("foo/foofoobar", "/home/rtc/bar/foofoobar~") = -1 EXDEV (Invalid cross-device link)
stat("foo/foofoobar", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
getxattr("foo/foofoobar", "system.posix_acl_access", 0x7fff095abaa0, 132) = -1 EOPNOTSUPP (Operation not supported)
open("foo/foofoobar", O_RDONLY)         = 4
open("/home/rtc/bar/foofoobar~", O_WRONLY|O_CREAT|O_EXCL|O_NOFOLLOW, 0100664) = 5
read(4, "", 8192)                       = 0
close(4)                                = 0
close(5)                                = 0
unlink("foo/foofoobar")                 = 0
fsync(3)                                = 0
open("foo/foofoobar", O_WRONLY|O_CREAT|O_TRUNC, 0664) = 4
fsync(4)                                = 0
getxattr("/home/rtc/bar/foofoobar~", "security.selinux", "unconfined_u:object_r:user_home_t:s0", 255) = 37
socket(PF_LOCAL, SOCK_STREAM|SOCK_CLOEXEC, 0) = 5
connect(5, {sa_family=AF_LOCAL, sun_path="/var/run/setrans/.setrans-unix"}, 110) = -1 ENOENT (No such file or directory)
close(5)                                = 0
getxattr("foo/foofoobar", "security.selinux", "system_u:object_r:fusefs_t:s0", 255) = 30
socket(PF_LOCAL, SOCK_STREAM|SOCK_CLOEXEC, 0) = 5
connect(5, {sa_family=AF_LOCAL, sun_path="/var/run/setrans/.setrans-unix"}, 110) = -1 ENOENT (No such file or directory)
close(5)                                = 0
setxattr("foo/foofoobar", "security.selinux", "unconfined_u:object_r:user_home_t:s0", 37, 0) = -1 EOPNOTSUPP (Operation not supported)
stat("foo/foofoobar", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
stat("foo/foofoobar", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
close(4)                                = 0
chmod("foo/foofoobar", 0100664)         = 0
stat("/home/rtc/foo/foofoobar", {st_mode=S_IFREG|0664, st_size=0, ...}) = 0
unlink("/home/rtc/bar/foofoobar~")      = 0
open(".", O_RDONLY)                     = 4
fchdir(4)                               = 0
chdir("foo")                            = 0
getcwd("/home/rtc/foo", 4096)           = 14
fchdir(4)                               = 0
close(4)                                = 0
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
write(1, "\r\r\nCould not set security context for foo/foofoobar\r\r\n\33[61;17H0L, 0C written\33[62;1H\33[K\33[62;1H\33[32mPress ENTER or type command to continue\33[?12l\33[?25h", 149) = 149
select(1, [0], NULL, [0], {4, 0})       = 0 (Timeout)
lseek(3, 0, SEEK_SET)                   = 0
write(3, "b0VIM 7.4\0\0\0\0\20\0\0T\367LR\31\0\0\0\n\30\0\0rtc\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0intertalk\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0~rtc/foo/foofoobar\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 4096) = 4096
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
select(1, [0], NULL, [0], {0, 0})       = 0 (Timeout)
select(1, [0], NULL, [0], NULL

The following can be seen:

1. The error is caused by a failure to set the security context. That is because fuse and thus sshfs don't support selinux context setting. vim has backupcopy set to "auto", and, in the example, the backup dir is on a different filesystem than the file to be overwritten. Yet, vim incorrectly assumes that the preconditions for backupcopy=no are met (it seems not to check whether filesystems are different), attempting to vim_rename() the file. The native rename fails, so vim_rename() falls back to copy-and-delete. vim then writes the file, sees that the context is not the same, and tries copying it, which fails.

2. vim tries to output an error message that does not show up on the screen ("Could not set security context for foo/foofoobar"). This seems problematic in itself.

3. vim never tries to get the security context of the original file, and never changes the security context of the temporary file accordingly. Yet, it later gets the security context of the temporary file and attempts to change the one of the newly written file. This can't be correct and seems to be a bug.

So how to solve this problem?

- I suppose the easiest solution is to add mch_copy_sec() to vim_rename(), which seems to be missing. The second mch_copy_sec() executed later on (to set the selinux context of the newly written file to that of the backup file) will then detect that the security context is unchanged. Things will then work fine, except for the opposite case: The backupdir being on an a device that doesn't allowing setting the security context, but the file to be overwritten having a security context. Perhaps, in this case, the message is justified, because the context is neccesarily lost for the backup file.

- I don't know how to fix that the error doesn't show up. Perhaps use EMSG() instead of MSG_PUTS() in mch_copy_sec()?

-  The proposed fix seems not to fix everything. Copying the file in vim_rename() obviously can fail to copy the security context, which is then lost (and the written file will even be left with the newly assigned security context of the backup copy, which may be a problem for selinux security). Why not just try a native rename() instead of vim_rename() in buf_write(), and if the rename fails for all backup dirs, continue as if backupcopy=yes? This would possibly change the behavior, however, for example in case backupdir contains two directories, the first working fine only with copying. An alternative would be to detect that filesystems differ and so predict a failing rename, assuming backupcopy=yes from the very beginning in this case. Yet another alternative would be to split mch_copy_sec() into mch_get_sec() and mch_put_sec(), so we don't have to rely on the the backup file to preserve the context.

Version-Release number of selected component (if applicable):
vim-enhanced-7.4.027-2.fc19.i686
vim-common-7.4.027-2.fc19.i686
vim-minimal-7.4.027-2.fc19.i686
vim-X11-7.4.027-2.fc19.i686
vim-filesystem-7.4.027-2.fc19.i686

How reproducible:
always

Steps to Reproduce:
1. mkdir ~/foo ~/bar
2. sshfs localhost:/tmp ~/foo
2. vim ~/foo/foofoobar
3. :se backupdir=~/bar/
4. :w
5. :w

Actual results:
"foo/foofoobar" 0L, 0C written
Press ENTER or type command to continue

Expected results:
"foo/foofoobar" 0L, 0C written

If there actually were a valid error, the expected result would be:
Could not set security context for foo/foofoobar
"foo/foofoobar" 0L, 0C written
Press ENTER or type command to continue


Additional info:
1. As one would expect, the problem goes away when you :set backupcopy=yes instead of backupcopy=auto

2. The problem doesn't occur if you are editing a file with (mode & umask) != mode. That is because one of the checks that backupcopy=auto performs is to see if open(...,mode) produces a file which has the given mode after stat, which is not the case if the umask is more strict. So whether you hit the bug actually depends on the file's mode. Pretty confusing, isn't it?

(3. There seems to be an "else" branch in buf_write() calling mch_copy_sec() twice. One call would be sufficient, I guess. This code is NOT in the execution path of the bug, however. I just noticed it while reading the source code.)