Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong temporary directory created following a reset via fs.os #965

Closed
vector-of-bool opened this issue Mar 8, 2024 · 4 comments · Fixed by #966
Closed

Wrong temporary directory created following a reset via fs.os #965

vector-of-bool opened this issue Mar 8, 2024 · 4 comments · Fixed by #966
Labels

Comments

@vector-of-bool
Copy link

Describe the bug
The /tmp created by _create_temp_dir may be invalid following modification of fs.os. _create_temp_dir consults tempfile.gettempdir() to find the default directory for temporary files. After the os module has been patched, the tempfile module attempts to probe the filesystem using the mock filesystem. Because the filesystem is reset by writing fs.os, tempfile fails to create probe files in the non-existent test directories, and gettempdir() returns root / as a fallback. The _create_temp_dir function will then create a symlink of /tmp -> /.

Background: By default, pyfakefs creates directories with mode 0o777, which is not a realistic mode for most directories, and especially not /. To test directory permissions, I apply os.chown('/', 0, 0); os.chmod('/', 0o755) to create a more "realistic" root directory that rejects writes.

With the merge of #961, the symlink of /tmp -> / becomes problematic after marking / to be non-writable for non-root users, as tempfile assumes that the temporary directory has mode 0o777.

How To Reproduce

def test_write_tmp(fs):
    # Mark '/' to be modifiable by only root
    os.chown("/", 0, 0)
    os.chmod("/", 0b111_101_101)
    with tempfile.TemporaryFile("wb"):
        pass

def test_write_tmp_after_reset(fs):
    # Set Linux as the filesystem OS
    fs.os = pyfakefs.fake_filesystem.OSType.LINUX
    # Mark '/' to be modifiable by only root
    os.chown("/", 0, 0)
    os.chmod("/", 0b111_101_101)
    with tempfile.TemporaryFile("wb"):
        pass

Your environment
Please run the following in the environment where the problem happened and
paste the output.

$ python -c "import platform; print(platform.platform())"
Linux-6.7.6-arch1-1-x86_64-with-glibc2.39
$ python -c "import sys; print('Python', sys.version)"
Python 3.11.7 (main, Jan 29 2024, 16:03:57) [GCC 13.2.1 20230801]
$ python -c "from pyfakefs import __version__; print('pyfakefs', __version__)"
pyfakefs 5.4.dev0
$ python -c "import pytest; print('pytest', pytest.__version__)"
pytest 7.2.0
@mrbean-bremen
Copy link
Member

Thanks, that's a good point!

As far as I can see, the default is 0o666 for regular files and 0x777 for directories, with the umask applied for files (but not always), and not applied for directories. This should not have a big impact (I hope), as most tests don't work with different users / groups.
I have to go through the code and check that the umask is always applied, and will leave the defaults as they are (making sure they are always applied).
As for the root dir - this is something I probably won't change by default, because it would break too many tests. I may think about an option to make the root path root-owned, which may change to the default in some major update later.

The temp directory creation has to be fixed of course.

@mrbean-bremen
Copy link
Member

Though I'll probably fix the temp dir problem only (and any other problems you encounter) for now, as the other changes may be a bit more complicated. Always applying the umask as I just tried breaks a few tests both in the fake and the real fs, so this is also not correct. I have to read up on permission handling first...

@mrbean-bremen
Copy link
Member

@vector-of-bool - as before: shall be fixed in main, please check!

@vector-of-bool
Copy link
Author

Thanks for the quick turnaround! Looks to be mostly fixed. I am hitting some issues with /tmp that may or may not be my own fault, but will require more investigation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants