Extend last_write_time implementation by special-casing file touching

[coroa]

Continuation of #1137 .

Fixes #1123 .

Only the owner (or a priviledged user) is allowed to set file modification time to a specific timestamp, as documented at the manpage for utimensat . Therefore, calling
last_write_time on the cache file fails with a RuntimeError: Operation not permitted exception.
But for anyone with write access to the file it is permitted to update the timestamp to the current time by passing a NULL-pointer or a UTIME_NOW special constant.

This special case of calling utimensat is not exposed by the std::filesystem::last_write_time layer. This PR adds a similar std:filesystem::now sentinel value to the last_write_time interface, which calls utimensat with the NULL pointer on supported systems and forwards the call to the original implementation on windows.

Fixes the test case described in #1123 (comment) :

❯ CONDA_PKGS_DIRS=/opt/mambaforge/pkgs /opt/mambaforge/envs/mamba-dev/bin/mamba create -n test2 python -v

Looking for: ['python']

info     libmamba Reading cache files '/tmp/tmpsbxx8053.*' for repo index 'installed'
info     libmamba Searching index cache file for repo 'https://conda.anaconda.org/conda-forge/linux-64/repodata.json'
info     libmamba Found cache at '/opt/mambaforge/pkgs/cache/497deca9.json'
info     libmamba No valid cache found
info     libmamba Expired cache (or invalid mod/etag headers) found at '/opt/mambaforge/pkgs'
info     libmamba Searching index cache file for repo 'https://conda.anaconda.org/conda-forge/noarch/repodata.json'
info     libmamba Found cache at '/opt/mambaforge/pkgs/cache/09cdf8bf.json'
info     libmamba No valid cache found
info     libmamba Expired cache (or invalid mod/etag headers) found at '/opt/mambaforge/pkgs'
info     libmamba Starting to download targets
[...]
info     libmamba Transfer done for 'conda-forge/linux-64'
info     libmamba Transfer finalized, status: 200
Channel: conda-forge[linux-64,noarch], platform: linux-64, prio: 1 : 0
Cache path:  /opt/mambaforge/pkgs/cache/497deca9.json
info     libmamba Reading cache files '/opt/mambaforge/pkgs/cache/497deca9.*' for repo index 'https://conda.anaconda.org/conda-forge/linux-64'
info     libmamba Writing SOLV file '497deca9.solv'
Channel: conda-forge[linux-64,noarch], platform: noarch, prio: 1 : 0
Cache path:  /opt/mambaforge/pkgs/cache/09cdf8bf.json
info     libmamba Reading cache files '/opt/mambaforge/pkgs/cache/09cdf8bf.*' for repo index 'https://conda.anaconda.org/conda-forge/noarch'
info     libmamba Writing SOLV file '09cdf8bf.solv'
info     libmamba Parsing MatchSpec python
info     libmamba Parsing MatchSpec python
info     libmamba Adding job: python
info     libmamba Problem count: 0
info     libmamba Found python version in packages to be installed 3.11.0
[...]

File modification times are updated as well:

❯ ls -l
total 362576
-rw-rw-r-- 1 usera cpt  74809545 Dec  1 00:27 09cdf8bf.json
-rw-rw-r-- 1 usera cpt  16766192 Dec  1 00:27 09cdf8bf.solv
-rw-rw-r-- 1 usera cpt 232431030 Dec  1 00:27 497deca9.json
-rw-rw-r-- 1 usera cpt  47261070 Dec  1 00:27 497deca9.solv

[coroa]

@wolfv Do I need to ping someone for review? I am pretty confident this solves the Permission denied errors, people in multi-user installations with ACLs or setgid or in some container setups have been reporting.

The gist is that on linux only the owner is allowed to update the modification time of a file to a particular timestamp, while it is permitted for everyone with write access to update the modification time to the current time, but the std::filesystem layer does not provide an interface for that.

[coroa]

Updated the PR description to make it easier to follow and hope to find someone for review!

[wolfv]

Thanks, I looked through the PR and it looks good to me! Will ask @Klaim what he thinks, then happy to merge! :)

Do you know if this also works on macOS?

[wolfv]

Judging by this issue on some other repo, it seems that the command was not available before macOS 10.12: bytecodealliance/rustix#157

[Klaim]

Thanks for the PR!

I just started reviewing this., other than informing myself of the details,
I'm trying to figure out if there isnt a way to do this without deferring to the toolchain's implementation of the standard's implementation to avoid future surprises or platforms not having utimensat. I'm not completely knowledgeable of the issue so bear with me ^^;

As the proposed function is an extension of the standard interface we try to keep, the overall idea of this PR seems reasonable to me.
At the moment I have a slight reticence to introducing fs::now because it's a name that could become problematic in the future - though we could also change it in a future version so I don't worry too much.

Could the utimensat implementation be restricted to non-windows but also non-apple platforms? Or are they also affected?

(I didn´t look at the ci issues yet)

[coroa]

I just started reviewing this., other than informing myself of the details, I'm trying to figure out if there isnt a way to do this without deferring to the toolchain's implementation of the standard's implementation to avoid future surprises or platforms not having utimensat. I'm not completely knowledgeable of the issue so bear with me ^^;

Great to see this is moving somewhere.

As the proposed function is an extension of the standard interface we try to keep, the overall idea of this PR seems reasonable to me. At the moment I have a slight reticence to introducing fs::now because it's a name that could become problematic in the future - though we could also change it in a future version so I don't worry too much.

I am not married to the name, if you find a more fitting alternative i'm happy to oblige.

Could the utimensat implementation be restricted to non-windows but also non-apple platforms? Or are they also affected?

There was no report about a multi-user issue on MacOS X, which given the nature of normal mac usage is not surprising, but i can actually try on my machine. Will report back.

On systems for which utimensat is not defined we could also defer back to utimes, but then we need an automatic build-system discovery step to differentiate between the two, i am wary of introducing this complexity but can put some research into it.

[Klaim]

Thanks for the details @coroa

I was looking at libcxx (from llvm) and libstdc++ (from gcc) and they actually do use utimensat by default when doing std::filesystem::last_write_time(path, std::filesystem::file_time_type::now(), ...) ...) (which is also what you proposed for the windows case.
When utimensat is not available (or not wanted) they switch to another implementation.

See:

• https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=libstdc%2B%2B-v3/src/filesystem/ops.cc;hb=b97c33fbd28e74a206c81c96a9b0b9fa3c8545d6#l919
• https://github.com/llvm/llvm-project/blob/main/libcxx/src/filesystem/operations.cpp#L1253 and https://github.com/llvm/llvm-project/blob/2e999b7dd1934a44d38c3a753460f1e5a217e9a5/libcxx/src/filesystem/filesystem_common.h#L532

Therefore, if my understanding is correct (I probably missed something), the issue is really that our calls to fs::last_write_time(path, now) should be rewritten to fs::last_write_time(path, std::filesystem::file_time_type::now()) systematically - or be replaced by your function with only the windows impl.

I cannot yet reproduce the initial issue, but if you do reproduce it, could you check if using only that implementation https://github.com/mamba-org/mamba/pull/2141/files#diff-14842e873613261b88cfd612f98b39a7ded38892a8465daa346d2b3ee62891f4R1165-R1166 for that new function actually fixes the issue?

[Klaim]

Looks like the CI issues are unrelated to this PR?

[wolfv]

yep, i think most of the ci issues are unrelated :)

[coroa]

Therefore, if my understanding is correct (I probably missed something), the issue is really that our calls to fs::last_write_time(path, now) should be rewritten to fs::last_write_time(path, std::filesystem::file_time_type::now()) systematically - or be replaced by your function with only the windows impl.

No, you are slightly off course.

fs::last_write_time(path, std::filesystem::file_time_type::clock::now()) creates a timestamp for the current time, then (in the llvm libcxx impl's operations.cpp) converts this to a timespec and then calls utimensat to set file modification and access times to this timespec. Unfortunately, this is a privileged operation where you have to own the file (or be root or similar). Instead you want to pass in a TimeSpec with the special value UTIME_NOW or a NULL pointer. The latter is what my implementation does.

If the tv_nsec field of one of the timespec structures has the special value UTIME_NOW, then the corresponding file timestamp is set to the current time. If the tv_nsec field of one of the timespec structures has the special value UTIME_OMIT, then the corresponding file timestamp is left unchanged. In both of these cases, the value of the corresponding tv_sec field is ignored.

(as described in the manpage utimensat )

[coroa]

I unfortunately CAN reproduce the issue on MacOS 12.6.

[coroa]

fs::last_write_time(path, std::filesystem::file_time_type::clock::now())

Note also that this is equivalent to the current implementation which exhibits the issue, since last_write_time is currently called with exactly this timestamp. Refer to

mamba/libmamba/src/core/subdirdata.cpp

Line 462 in 01f6018

auto now = fs::file_time_type::clock::now();

, and

mamba/libmamba/src/core/subdirdata.cpp

Line 512 in 01f6018

fs::last_write_time(solv_file, now);

[coroa]

I found that llvm's libcxx implementation uses the presence of the UTIME_NOW constant to decide whether use utimensat.

https://github.com/llvm/llvm-project/blob/2e999b7dd1934a44d38c3a753460f1e5a217e9a5/libcxx/src/filesystem/filesystem_common.h#L42-L44

I would propose to make the new code conditional on this constant being defined, then we fall back to the old implementation when utimensat does not exist and use the fixed code on newer systems?

[Klaim]

I would propose to make the new code conditional on this constant being defined, then we fall back to the old implementation when utimensat does not exist and use the fixed code on newer systems?

That makes sense to me indeed.

If that fixes also the macos build, then after fixing the ci I'm ok to merge this change 👍🏽 Thanks for the detailed explaination.

[coroa]

I was wondering whether to prefix the constant, ie like _MAMBA_USE_UTIMENSAT?

[coroa]

Ok, with this change it compiles and fixes the issue on ubuntu and MacOSX. Tests on MacOSX pass, on ubuntu i'm seeing some file locking errors, which i don't think are related to the filesystem layer.

(Merry christmas!)

[wolfv]

Thanks so much! I hope you had some wonderful holidays :)