Fallback for cross-device link error during rename call

[kubatek94]

Few issues (#16306, #14743, #10563) have been raised with a cross-device link error.

These can be reproduced within Docker while using volume mounts. When two folders from a single volume/device are mounted within the container and user tries to move folders between those two mounted volumes, the rename PHP function is called, but the underlying overlayfs driver returns an error, which is then triggered as warning by PHP.

According to Docker documentation (https://docs.docker.com/storage/storagedriver/overlayfs-driver/#modifying-files-or-directories), this case should be handled within the application and in case of the error, we should attempt a "copy and rename" strategy.

This PR addresses the issue. I tried to make it so that it handles the cross-device link error, but forwards unexpected warnings to the original error handler to make it rather transparent in case of other failures.

I'm open to any feedback :)

[kesselb]

Thanks 👍

Your approach to solve this problem is very nice from a software design perspective but also a bit more complex than necessary.

https://github.com/symfony/symfony/blob/85f793bec65c54bce9cb2b8352ebe45b5a2313df/src/Symfony/Component/Filesystem/Filesystem.php#L273-L282

That's simple. Try to rename and use copy & unlink on failure. If rename failed with a warning regarding free space it's probably dump to try a copy again.

Let's see what the other reviewers are thinking ;)

[kubatek94]

Thanks for your feedback @kesselb :) Happy to see it so early :)

I agree that the solution is more complex than the one you've linked to. The question is whether that complexity is necessary.

I tried to create a solution that is least invasive. Even if it's more complex, that complexity is contained to a separate small class and can be looked at in isolation.

My assumption was that we want to keep this method backwards compatible and only fix this specific "cross device link" issue. If we suppress the warnings with the "@" and return false from rename, that will mask some issues, such as permission issues (please see the unit tests to see what I mean). If instead of returning false we throw an exception, we break the contract of that method and any callers which might only expect to get boolean result from that method call. Throwing an exception has much different behaviour than currently warnings being triggered, as for warnings the execution continues.

If we suppress the warnings with "@" we also suppress all the warnings and they won't be logged. If solution like this was here in the first place, it might have never been reported, since there would be nothing in the logs. The "rename" would simply return false for some reason.

These are some arguments that I find against just suppressing warnings with "@" in this case. This lead me to the use of custom temporary error handler, as a viable way of solving it, while meeting the assumptions outlined above.

[icewind1991]

Wouldn't using the $stat1['dev'] !== $stat2['dev'] check in more places be enough to have this or is this check not enough in all cases?

Trying to match error message meant for human consumption seems fragile.

[kubatek94]

@icewind1991 Thanks for feedback.

The check you mentioned unfortunately doesn't work in this case, because the folders could be physically on the same device. I've checked and the 'dev' property in both cases is the same.

I understand your concerns with basing the logic based on the error message. It's not something I would do normally. The issue is that those errors actually come from native libc functions. In this example, the error comes from the native rename function. Those functions normally set the errno to non zero code to indicate failure and provide more details. The issue is that we cannot read this variable from PHP land - I checked both posix_get_last_error and error_get_last and they return 0 and NULL respectively after the rename fails.

In other words, setting an error handler to check the error message for the string of interest is the only solution I found that works and doesn't have massive drawbacks.

The only alternative I see is potentially changing this block of code:

if ($this->is_dir($path1)) {
	// we can't move folders across devices, use copy instead
	$stat1 = stat(dirname($this->getSourcePath($path1)));
	$stat2 = stat(dirname($this->getSourcePath($path2)));
	if ($stat1['dev'] !== $stat2['dev']) {
		return $this->copyAndUnlink($path1, $path2);
	}

	if ($this->treeContainsBlacklistedFile($this->getSourcePath($path1))) {
		throw new ForbiddenException('Invalid path', false);
	}
}

to this:

if ($this->is_dir($path1)) {
	if ($this->treeContainsBlacklistedFile($this->getSourcePath($path1))) {
		throw new ForbiddenException('Invalid path', false);
	}

	return $this->copyAndUnlink($path1, $path2);
}

This would always perform copy and unlink instead of rename if we're trying to move a directory. This obviously has a cost because most cases may not need it, it takes more time than rename, isn't atomic and requires double the space to perform the operation.

What do you think is a way forward?

[icewind1991]

I think my preferred option would be to always fall back to copy+remove if renaming fails as Flysystem does and @kesselb link above.

While this would have some "false positives" retrying, but seems more reliable then relying on the error message. (which might differ based on the libc used in the system)

[Capusjon]

I tried @kubatek94 fix. Just putting the PHP files in the right folder and for now this seems to have solved the error issue.

Can we expect this as a fix in a next installment or are there still bugs to flatten before implementing? Should I take some precautions before using these php files?

Kind regards!

[Capusjon]

I don't know if I'm doing it right but, since I noticed the local.php file has changed since this pull request I tried to put both files together..

You can see it in the attachment for easy(?) reference
Local.txt

[Capusjon]

any update on this? What needs to be done to implement/fix this issue?

I'll help if I'm able :)

[szaimen]

@kubatek94 since you started the pull request, may I ask what is needed here to get this in?
Would love to see the trashbin working correctly, again.

[ckolumbus]

Same issue still here with dockerized Nextcloud 19.0.6... any chance that this fix makes it in?
Update: ... and reproduced this after upgrading to 20.0.4.. so it's still relevant i guess

[come-nc]

Main question is of course is this still needed? I see we have a conflict on Local.php.

[come-nc]

Why $path1 and $path2 are not parameters of the fallback callable?

[come-nc]

This is quite complicated to get the errors from rename, is there any library outthere implementing something like this that we can use instead?

Or maybe we can just try copyAndUnlink when rename returns false as @icewind1991 suggested.

A potential problem arise when the copy works and not the unlink though…

[kesselb]

Closing the pull request due to inactivity.