Use a temporary file for writes

[kirawi]

Supersedes #4276 and resolves #3967

I initially suspected it had to do with not seeking to the beginning of the file, but it turned out to be something equally as dumb:

• rename only changes the path of the open inode to the new path, so any open handles to the old inode are not affected. Thus, I had to reopen the temporary file after each write.
• rename can replace readonly files if the directory has write permissions. So failing if the file at the path is readonly remains the last task for the tests to pass. Actually, it would be good to also apply the same permissions and everything to the new file as well.

Bear in mind I was unaware of even what an inode was until 5 minutes ago, so I might be wrong.

[pascalkuthe]

I.plementing this correctly is going to be a lot harder. If you create a new temporary field all file metadata (timestamps, permissions, owner, groups,..) need to be copied to the new file. That code is highly platform dependent (not supported by std) and not easy to implement

[kirawi]

Gotcha, I'll look into it. I think that the only thing that is platform-dependent is owners/groups and permissions. Unix seems simple, I don't know about Windows though (Vim doesn't try for Windows with its undofiles).

[kirawi]

I'll be punting this to the backburner for now. I'd have to work with winapi for the aforementioned metadata which would require me to learn unsafe Rust.

[kirawi]

How would you feel about restricting this to only Unix systems? I wrote code to copy permissions on Windows:

fn chown(from: &Path, to: &Path) -> std::io::Result<()> {
    use std::os::windows::io::AsRawHandle;

    // CHECK: Requires read
    let handle = HANDLE(std::fs::File::open(from)?.as_raw_handle() as isize);
    let mut owner = PSID::default();
    let mut group = PSID::default();
    unsafe {
        windows::Win32::Security::Authorization::GetSecurityInfo(
            handle,
            SE_FILE_OBJECT,
            OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION,
            Some(&mut owner),
            Some(&mut group),
            None,
            None,
            None,
        )?;
    }

    // CHECK: requires write
    let handle = HANDLE(std::fs::File::open(to)?.as_raw_handle() as isize);
    unsafe {
        windows::Win32::Security::Authorization::SetSecurityInfo(
            handle,
            SE_FILE_OBJECT,
            OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION,
            owner,
            group,
            None,
            None,
        )?;
    }

    Ok(())
}

But aside from the questionable safety of what I've written, I'd rather not have to maintain it since this kind of stuff is sparsely documented in the API.

[pascalkuthe]

I took a look at what vim does... It's not easy tgrwap but what I found is that:

• copying the temporary file starts here https://github.com/vim/vim/blob/b21499537fb9fd0ff407e6113ac60ebd82058e2a/src/bufwrite.c#L1457
• you can use set_permssion to cover the chmod/set_perm stuff. On UNIX make sure to change the owner before changing permissions.
• I think there are some libraries for setting timestaps
• the windows access control stuff is handled by two functions: https://github.com/vim/vim/blob/b21499537fb9fd0ff407e6113ac60ebd82058e2a/src/os_win32.c#L4312 and https://github.com/vim/vim/blob/b21499537fb9fd0ff407e6113ac60ebd82058e2a/src/os_win32.c#L4224
• for low level code like this the c code is fairly similar to rust you can mostly copy these two functions
• the only thing that will be different is how you open the file/obtain the handle. You got mostly the right idea here you just need to make sure that the File is stored in its own let binding otherwise the handle will get closed.
• both forw windows and UNIX there are some gnarly details around extra file metadata (selinux, extra attributes, ntfs streams on windows...) we sadly will have to deal with all of that eventually but that is pretty niche and can wait for future work

[pascalkuthe]

the integration tests fail for me locally toso its not just a permission issue in the runner

[pascalkuthe]

I am not sure if you are quite using tmp file correctly. The following two caveats seem extremely relevant:

    /// Note: Temporary files cannot be persisted across filesystems. Also
    /// neither the file contents nor the containing directory are
    /// synchronized, so the update may not yet have reached the disk when
    /// `persist` returns.
    ///
    /// # Security
    ///
    /// Only use this method if you're positive that a temporary file cleaner
    /// won't have deleted your file. Otherwise, you might end up persisting an
    /// attacker controlled file.

I am pretty sure mky tmpfiles are in a separate btrfs partition so that's why its not working

[pascalkuthe]

I think you will want to use https://docs.rs/tempfile/latest/tempfile/struct.Builder.html#method.tempfile_in to create the tomprary file in the same dirtory as the destination file. To avoid creating a bunch of random files you can set the prefix to the filename (including extension) and the suffix to bak so its clear we are writing temporary backup files

[pascalkuthe]

I don't think the chown stuff ist actually the problem. We ignore errors so I don't see how that could block writing the file

[pascalkuthe]

and two test failures are simple bugs in the testing infrastructure keeping the file handle open and hence reading from the closed file

[pascalkuthe]

ok I did those changes myself (hope that's ok its separate commits so nothing was lost) locally integration tests pass let's see if CI is green. I think this should handle the subtleties for the most part

[kirawi]

I can test on my Windows machine since I can replicate it in integration tests, if you'd like? I'm also trying to debug it on my end.

[kirawi]

sync_all on Windows calls FlushFileBuffers, which shouldn't fail if the handle has write permissions. However, it does note that: To flush all open files on a volume, call FlushFileBuffers with a handle to the volume. The caller must have administrative privileges. For more information, see [Running with Special Privileges](https://learn.microsoft.com/en-us/windows/desktop/SecBP/running-with-special-privileges).

[pascalkuthe]

yeah I had to stop working due to a meeting. I changed the code to move the old file to a backup and then write to the intended path (backup file is only deleted on successful write). This is how vim handles this and should remove the permission concerns from copy_metadata (since we won't need to move the file afterwards it doesn't matter if we don't have permissions to access it)

I believe that the code is correct now and the integration tests just fail because of some tests bugs. Potentially the sync_all call. It does seem to be panicking there. Do we really need to call that?

[pascalkuthe]

ah looks like you fixed it already, great work! I undid my debug changes which made the CI fail and I noticed that we should be flusing/syncing the old file should not actually be needed since we are replacing the file (so a flush on the old file won't do much good).

Lets' see if this passes CI it works locally at least (on linux but seeing that you fixed windows CI I doubt that this would regress that)

[pascalkuthe]

if CI passes we just need to log (with log::error) all the IO error we are currently ignoring then I think this should be good to go

[kirawi]

Before this is ready to merge, I want to port the faccess code for Windows to windows-sys and make the logic match since I used ? where it shouldn't be used according to the original logic.

[pascalkuthe]

yeah in ead of bailing out on error we should just log all of them with log::errors. Using windows-sys would be great 👍 Thanks for working on the low level stuff that is pretty gnarly.

One thing vim does that we don't is that w! will turn a readonly file into a writable one if possible. We should do that too by ignoring the readonly check and removing readonly form the permissions in that case

[pascalkuthe]

its not critical to handle the readonly stuff in this PR since we didn't do anything about that currently either so if that turns out to be more complicated then just leave it for future work

[pascalkuthe]

(rebased as there were just some minor conflicts in the integration tests)

[pascalkuthe]

(ups)

[the-mikedavis]

Nice work @kirawi & @pascalkuthe !