Fix conan cache restore crashing with PermissionError when the cache contains read-only files - #20254
Fix conan cache restore crashing with PermissionError when the cache contains read-only files#20254danimtb wants to merge 4 commits into
conan cache restore crashing with PermissionError when the cache contains read-only files#20254Conversation
…e cache contains read-only files
|
The failing linux tests look unrelated to the changes in this PR https://github.com/conan-io/conan/actions/runs/31588322506/job/94108647641?pr=20254 |
| # Extraction opens the destination for writing, read-only files would fail | ||
| dest = os.path.join(cache_folder, member.name) | ||
| if member.isfile() and os.path.isfile(dest) and not os.access(dest, os.W_OK): | ||
| os.chmod(dest, os.stat(dest).st_mode | stat.S_IWRITE) |
There was a problem hiding this comment.
It doesn't end up with the same permissions it had before. Is this expected?
There was a problem hiding this comment.
Exactly, this seems to be forcing some permissions that it didn't have before?
There was a problem hiding this comment.
The chmod is temporary and only makes the existing destination writable so extractall() can open it. The tarfile then applies the mode stored in the archive member and file ends with the same permissions it had when saved.
I will improve the tests to reflect this, thanks for pointing it out!
| for member in the_tar.getmembers(): | ||
| # Extraction opens the destination for writing, read-only files would fail | ||
| dest = os.path.join(cache_folder, member.name) | ||
| if member.isfile() and os.path.isfile(dest) and not os.access(dest, os.W_OK): |
There was a problem hiding this comment.
The change is only applied to the files. Could there be a directory with write-only permissions that ends up causing a similar error?
memsharded
left a comment
memsharded
left a comment
There was a problem hiding this comment.
I am not sure about this. If some files are read-only, they are read-only, they cannot be overwritten or changed permissions. It is the responsibility of the user to handle those files.
| # Extraction opens the destination for writing, read-only files would fail | ||
| dest = os.path.join(cache_folder, member.name) | ||
| if member.isfile() and os.path.isfile(dest) and not os.access(dest, os.W_OK): | ||
| os.chmod(dest, os.stat(dest).st_mode | stat.S_IWRITE) |
There was a problem hiding this comment.
Exactly, this seems to be forcing some permissions that it didn't have before?
…b/conan into danimtb/cache-restore-permissions
4 participants
Changelog: Bugfix: Fix
conan cache restorecrashing withPermissionErrorwhen the cache contains read-only files.Docs: Omit
developbranch, documenting this one.Fixes #20241
There are two different points where this happens, and the second one was only reachable
after fixing the first:
tarfile.extractall()opens each destination file inwbmode, which fails when the filealready exists and is read-only. This is the traceback reported in the issue, and it happens
when the package folder is restored in place (a package downloaded from a server, or an
archive restored over the same cache it was saved from).
shutil.rmtree()is used to remove a previously restored package/metadata folder when thepackage has to be relocated to a different folder in the destination cache. It has no
handler for read-only files, so it raises
PermissionError: [WinError 5] Access is denied.The fix makes pre-existing destination files writable before extracting, preserving the rest
of the permission bits so group/other permissions are not dropped on Linux and macOS. Files
keep the mode stored in the archive after extraction, so read-only files stay read-only.
There is a pull-request opened some days ago at #20249 but it misses the second issue mentioned in this one plus some tests that reproduce the issue, so I'd rather open this new one.