search

kd2org / karadav

Lightweight NextCloud compatible WebDAV server
https://fossil.kd2.org/karadav/
GNU Affero General Public License v3.0
155 stars 14 forks source link

Copy and unlink instead of rename. #18

Closed ameyp closed 2 years ago

ameyp commented 2 years ago

Rename tries to call chown, which causes issues on network-mounted filesystems like NFS.

bohwaz commented 2 years ago

What kind of issues?

Unfortunately I cannot accept this PR as it is, as rename is atomic, but copy is not. Meaning that with rename a client will either get the old file or the new file, but always a full file, while with copy the client might get an incomplete and corrupt file.

For example if you transfer a 20 GB file, it will take a while to copy, and any client downloading it during that will get an incomplete file, without knowing it.

So this patch would corrupt files for most users.

There should be a better solution for NFS than using copy? as it will create all sorts of problems.

Or if there are no solutions, this should be made an option.

ameyp commented 2 years ago

Ah, fair, I did not consider that. The issue I ran into, is that the rename prints a warning saying Invalid argument. The move still succeeds, so perhaps @rename would be a better alternative to this current PR?

bohwaz commented 2 years ago

Ah, fair, I did not consider that. The issue I ran into, is that the rename prints a warning saying Invalid argument. The move still succeeds, so perhaps @rename would be a better alternative to this current PR?

I don't think so as it's just hiding a problem with your NFS configuration I think. This is documented here: https://stackoverflow.com/a/67873237

If you cannot change the NFS config, the best option is probably to change this PR to add an option in config.dist.php to do copy+unlink instead of rename :)

ameyp commented 2 years ago

ID mapping is how NFS works, as far as I know. None of the Unix CLI tools complain about permissions, I don't know why PHP wants to chown a file it has successfully created.

Anyway, I spent several hours yesterday trying to work around another issue I ran into that is probably related to my rewrite rule in nginx, and decided that it wasn't worth it. I'm just going to close this PR.

ameyp commented 2 years ago

I was wrong, there indeed is an issue with my NFS setup, IDs are not being mapped correctly. I'm still miffed that PHP complains about it (because it still has permissions to create the file that it's complaining about) but I might as well fix my setup. Thank you for pointing that out!

bohwaz commented 2 years ago

Great!

Looking at the PHP core source code, it is actually doing a copy + unlink when rename() failed because the underlying filesystem doesn't allow it (eg. FAT), that's why you are getting an error: making a copy instead of a rename requires to set chown/chmod on the new file.

This is detailed here: https://www.php.net/manual/en/function.rename.php#117590

So I guess that adding a @ in front of @rename should be a good solution as well as PHP is already trying its best copy the file instead of renaming if it is required.

ameyp commented 2 years ago

Thanks for the explanation with the deep dive into core code! I fixed my nfs mount to map ids correctly, so I don't think my change is required after all