Gvisor failed to ftruncate on deleted file

[DarcySail]

Description

When a GR3 program open a file and delete(unlink) it, then ftruncate the deleted file with FD, the ftruncate syscall will fail with 'Invalid argument'.

In function Tsetattr.handle explains the behavior, but I don't really understand it.

func (t *Tsetattr) handle(cs *connState)
// We don't allow setattr on files that have been deleted.
// This might be technically incorrect, as it's possible that
// there were multiple links and you can still change the
// corresponding inode information.
if ref.isDeleted() {
    return syscall.EINVAL
}

Steps to reproduce

Run the following c code:

#include<stdlib.h>
#include<unistd.h>
#include<stdio.h>
#include<fcntl.h>
#include <sys/types.h>
#include <errno.h>
#include <string.h>

int main()
{
    int file_desc = open("test.txt",O_RDWR|O_CREAT|O_EXCL|O_NOFOLLOW, 0600);
    printf("file_desc: %d\n", file_desc);

    int ret = unlink("test.txt");
    printf("unlink: %d\n", ret);

    int ftruncate_ret = ftruncate(file_desc, 0);
    printf("ftruncate_ret: %d\n", ftruncate_ret);

    return 0;
}

How we found above syscall sequence:
We simply run pytest --version in runsc.

#pouch run --rm --net host --runtime=runsc -ti python bash -c 'pip install pytest && pytest --version'

Collecting pytest
  Downloading pytest-6.0.1-py3-none-any.whl (270 kB)
     |████████████████████████████████| 270 kB 22 kB/s
Collecting more-itertools>=4.0.0
  Downloading more_itertools-8.4.0-py3-none-any.whl (43 kB)
     |████████████████████████████████| 43 kB 57 kB/s
Collecting pluggy<1.0,>=0.12
  Downloading pluggy-0.13.1-py2.py3-none-any.whl (18 kB)
Collecting packaging
  Downloading packaging-20.4-py2.py3-none-any.whl (37 kB)
Collecting attrs>=17.4.0
  Downloading attrs-19.3.0-py2.py3-none-any.whl (39 kB)
Collecting py>=1.8.2
  Downloading py-1.9.0-py2.py3-none-any.whl (99 kB)
     |████████████████████████████████| 99 kB 51 kB/s
Collecting toml
  Downloading toml-0.10.1-py2.py3-none-any.whl (19 kB)
Collecting iniconfig
  Downloading iniconfig-1.0.1-py3-none-any.whl (4.2 kB)
Collecting pyparsing>=2.0.2
  Downloading pyparsing-2.4.7-py2.py3-none-any.whl (67 kB)
     |████████████████████████████████| 67 kB 46 kB/s
Collecting six
  Downloading six-1.15.0-py2.py3-none-any.whl (10 kB)
Installing collected packages: more-itertools, pluggy, pyparsing, six, packaging, attrs, py, toml, iniconfig, pytest
Successfully installed attrs-19.3.0 iniconfig-1.0.1 more-itertools-8.4.0 packaging-20.4 pluggy-0.13.1 py-1.9.0 pyparsing-2.4.7 pytest-6.0.1 six-1.15.0 toml-0.10.1
pytest 6.0.1
Traceback (most recent call last):
  File "/usr/local/bin/pytest", line 8, in <module>
    sys.exit(console_main())
  File "/usr/local/lib/python3.8/site-packages/_pytest/config/__init__.py", line 180, in console_main
    code = main()
  File "/usr/local/lib/python3.8/site-packages/_pytest/config/__init__.py", line 165, in main
    config._ensure_unconfigure()
  File "/usr/local/lib/python3.8/site-packages/_pytest/config/__init__.py", line 920, in _ensure_unconfigure
    fin()
  File "/usr/local/lib/python3.8/site-packages/_pytest/capture.py", line 632, in stop_global_capturing
    self._global_capturing.pop_outerr_to_orig()
  File "/usr/local/lib/python3.8/site-packages/_pytest/capture.py", line 522, in pop_outerr_to_orig
    out, err = self.readouterr()
  File "/usr/local/lib/python3.8/site-packages/_pytest/capture.py", line 563, in readouterr
    out = self.out.snap()
  File "/usr/local/lib/python3.8/site-packages/_pytest/capture.py", line 484, in snap
    self.tmpfile.truncate()
OSError: [Errno 22] Invalid argument

Environment

Please include the following details of your environment:

• Latest runsc version, on master branch.
• Linux k08j13247.eu95sqa 4.9.151-015.ali3000.alios7.x86_64 Nginx does not run #1 SMP Tue Mar 12 19:10:26 CST 2019 x86_64 x86_64 x86_64 GNU/Linux

[DarcySail]

@tanjianfeng @ianlewis

[DarcySail]

Hi @amscanne Adin, I notice you introduce these logic in following patch, would you please
give a specific case which will fail if we don't do so?

commit 75cd70e
Author: Adin Scannell [email protected]
Date: Tue Oct 23 00:19:11 2018 -0700

Track paths and provide a rename hook.

This change also adds extensive testing to the p9 package via mocks. The sanity
checks and type checks are moved from the gofer into the core package, where
they can be more easily validated.

PiperOrigin-RevId: 218296768
Change-Id: I4fc3c326e7bf1e0e140a454cbacbcc6fd617ab55

[tanjianfeng]

Anyone can add some background here? As there's no description on the orginal problem of this change, we cannot blindly revert that change.

[majek]

Using repro C program from this ticket. Under gvisor release-20200907.0-44-g7e0b019b84b9:

runsc:~# ./a.out
file_desc: 3
unlink: 0
ftruncate_ret: -1

Outside gvisor on native linux:

$ ./a.out 
file_desc: 3
unlink: 0
ftruncate_ret: 0

I've stumbled on this problem months ago but didn't bother to file a ticket. I don't remember the exact circumstances but it did bite me before.

[amscanne]

Re: 75cd70e, that change is a defense-in-depth measure. It prevents a compromised Sentry from exploiting walk races. It is not strictly necessary, given there are a number of other defense-in-depth measures in place (e.g. gofers run in a chroot, which makes the races largely irrelevant), but I don't think it hurts.

Is the only thing required here relaxing the constraint in the handler? That seems fine to me, it was probably overreaching.

[dean-deng]

We may have problems if we allow the operation on a deleted file, if another file with the same name has been created. An open FID does not necessarily correspond to an open FD in every gofer implementation--some implementations may still walk the path again despite having an open FID. Therefore, we still need this check so that the new file is not truncated.

One way to solve this would be to add a new setattr call that requires the operation to be performed on an open file. This would be useful for a few system calls, e.g. ftruncate, fchmod, utimensat. Gofers that don't support this could simply fail the call if the file has been unlinked.

[DarcySail]

We may have problems if we allow the operation on a deleted file, if another file with the same name has been created. An open FID does not necessarily correspond to an open FD in every gofer implementation--some implementations may still walk the path again despite having an open FID. Therefore, we still need this check so that the new file is not truncated.

One way to solve this would be to add a new setattr call that requires the operation to be performed on an open file. This would be useful for a few system calls, e.g. ftruncate, fchmod, utimensat. Gofers that don't support this could simply fail the call if the file has been unlinked.

You're right, though I delete the if ref.isDeleted() check, it still fails when try to write to file with same name .

#include<fcntl.h>
#include<stdio.h>
#include<unistd.h>

int main()
{
    int file_desc = open("test.txt",O_RDWR|O_CREAT|O_EXCL|O_NOFOLLOW, 0600);
    printf("file_desc: %d\n", file_desc);

    int ret = unlink("test.txt");
    printf("unlink: %d\n", ret);

    int file_desc2 = open("test.txt",O_RDWR|O_CREAT|O_EXCL|O_NOFOLLOW, 0600);
    printf("file_desc2: %d\n", file_desc2);

    int wn = write(file_desc2, "fff", 3);
    printf("wn: %d\n", wn);

    int ftruncate_ret = ftruncate(file_desc, 0);
    printf("ftruncate_ret: %d\n", ftruncate_ret);

    return 0;
}

[dean-deng]

Are you still getting EINVAL, or is it something else like ENOENT?

[YaroslavLitvinov]

Hello,

We faced problems running sqlite3 in gvisor environment. A substantial amount of sqlite3 db tests are failing on the latest gvisor, as sqlite3 calls unlink at the very beginning of session when working with temporary entities.

If you are interested, you can run a docker I prepared, note that the --vfs2 runsc flag is set:

docker pull yaroslavlitvinov/public:sqlite3.test
docker run --runtime runsc -it --entrypoint /bin/bash yaroslavlitvinov/public:sqlite3.test
./testfixture ../test/tempdb2.test

According to docs: a file does exist until the the file descriptor is not released.

If the name was the last link to a file but any processes still have the file open, the file will remain in existence until the last file descriptor referring to it is closed.

[fvoznika]

Thanks for the easy repro. The fsgofer included with runsc either reuses the open FD or safely reopens using the magic link in proc/[pid]/fd, so it's safe to remove the check. I'll take a look...