Fix for systems where the RecursiveDirectoryIterator does not behave as specified

[caco3]

superseded by #515

---

This PR solves the failing updates due to issues on removing the old files and copying the new files.
See #158 for details.
And it also solves the issue some systems have when modifying a directory while iterating through it, see #509 for details.

TL;DR

On some servers (notable FreeBSD 12.4 on hostspoint.ch), the updater tries to rmdir() non-empty folders. This is not supported on PHP. The updater already has a function to recursively delete (recursiveDelete()), how ever it is not used consistently.

Additionally, the iterator gets gets messed up in the move step when modifying the directories. Thus we copy() instead of rename() and remove the whole folders/files afterwards in a separate iteration.

Scroll down for the fix.

Issue

Updater fails to delete old files:

updater log on first deletion failure:

2023-10-23T00:06:59+0200 xGP6zzWOT0 [info] startStep("9")
2023-10-23T00:06:59+0200 xGP6zzWOT0 [info] deleteOldFiles()
2023-10-23T00:07:25+0200 xGP6zzWOT0 [info] config sample exists
2023-10-23T00:07:25+0200 xGP6zzWOT0 [info] themes README exists
2023-10-23T00:07:25+0200 xGP6zzWOT0 [error] POST request failed with other exception
2023-10-23T00:07:25+0200 xGP6zzWOT0 [error] Exception: Exception
Message: Could not rmdir: /home/xxx/www/nextcloud/updater/../lib/l10n
Code:0
Trace:
#0 /home/xxx/www/nextcloud/updater/index.php(1388): Updater->deleteOldFiles()
php/php-src#1 {main}
File:/home/xxx/www/nextcloud/updater/index.php
Line:918

The general issue seems to be that rmdir only is able to delete empty folders.
The first time the issue occurs for me is on deleting the lib/l10n folder.
Looking on the folder, it clearly is not empty at that time:

> ls
af.json    az.js      br.js      el.js      es_AR.json es_EC.json es_PR.json fo.js      gd.json    hy.js      id.json    km.js      lb.js      mk.json    nl.json    pt_BR.js   si.js      th.js      vi.json
an.js      az.json    bs.js      el.json    es_CL.js   es_HN.json es_UY.json fo.json    he.js      hy.json    is.json    kn.js      lb.json    ms_MY.js   oc.js      pt_BR.json sk.js      th.json    zh_CN.js
ar.js      be.js      cs.json    eo.js      es_CL.json es_MX.json et_EE.json fr.js      hr.json    ia.js      it.js      kn.json    lo.json    nb.json    pl.js      pt_PT.js   sq.js      tk.json    zh_TW.js
ast.js     bg.json    da.js      eo.json    es_CO.js   es_NI.json eu.json    fr.json    hu.js      ia.json    ja.js      ko.js      lt_LT.json ne.json    pl.json    pt_PT.json sq.json    ur_PK.js
ast.json   bn_BD.js   de_DE.json es_419.js  es_DO.js   es_PE.js   fi.json    gd.js      hu.json    id.js      ka_GE.js   ko.json    lv.json    nl.js      ps.js      sc.json    ta.json    ur_PK.json

Note that it still contains 93 files. Odly, originally it contained 200 files, so some successed to get deleted...

Fix

• Validate return codes of rmdir() and unlink().
• do not loop directly through the iterator but generate a file list and iterate through that one (on BSD it seems that the iterator gets messed up when the directory gets modified)
• make log messages consistent
• minor fixes

Using that fix, I am successful to update Nextcloud 27.0.0 to the latest (currently 27.1.2).

[caco3]

@kesselb Please have a look on this PR which replaces #508

[caco3]

@solracsf @MorrisJobke @nickvergessen @blizzz @come-nc

Whoever is in charge, please review and provide further information how to pass all checks.
Also it strikes me that the updater.phar is part of the repo. Shouldn't this file be auto generated using a Github action?
Like in this PR, it does not get updated.

[Altahrim]

Hello,

Thank you for the fix!

I tried to reproduce issue php/php-src#509 but failed.
I am wondering if we could preload the file list. Something like:

	/**
	 * Moves the specified files except the excluded elements to the correct position
	 *
	 * @throws \Exception
	 */
	private function moveWithExclusions(string $dataLocation, array $excludedElements): void {
		// Build file list first, so the renames won't mess with it
		/** @var array<string, \SplFileInfo> */
		$fileList = iterator_to_array($this->getRecursiveDirectoryIterator($dataLocation), true);
		foreach ($fileList as $path => $fileInfo) {
			$fileName = explode($dataLocation, $path)[1];
			$folderStructure = explode('/', $fileName, -1);

Does it fix the issue for you? I think it will be faster if we avoid disk accesses.

[kesselb]

Following #509 (comment) the actual issue seems that the recursive directory iterator does not work properly on FreeBSD.

Given that we don't list FreeBSD as supported operating system https://docs.nextcloud.com/server/latest/admin_manual/installation/system_requirements.html I suggest closing this pull request and the related issue.

A user can always use the manual update.

[kesselb]

@caco3 mentioned here #508 (comment) that recursiveDelete works.

updater/lib/Updater.php

Lines 776 to 816 in 55eca48

	/**
	* Recursively deletes the specified folder from the system
	*
	* @throws \Exception
	*/
	private function recursiveDelete(string $folder): void {
	if (!file_exists($folder)) {
	return;
	}
	/** @var iterable<\SplFileInfo> $iterator */
	$iterator = new \RecursiveIteratorIterator(
	new \RecursiveDirectoryIterator($folder, \RecursiveDirectoryIterator::SKIP_DOTS),
	\RecursiveIteratorIterator::CHILD_FIRST
	);
	$directories = [];
	$files = [];
	foreach ($iterator as $fileInfo) {
	if ($fileInfo->isDir()) {
	$directories[] = $fileInfo->getRealPath();
	} else {
	if ($fileInfo->isLink()) {
	$files[] = $fileInfo->getPathName();
	} else {
	$files[] = $fileInfo->getRealPath();
	}
	}
	}
	foreach ($files as $file) {
	unlink($file);
	}
	foreach ($directories as $dir) {
	rmdir($dir);
	}
	$state = rmdir($folder);
	if ($state === false) {
	throw new \Exception('Could not rmdir ' . $folder);
	}
	}

A possible explanation could be that we iterator over the iterator before changing the file system.
While here we modify the file system while iterating.

[caco3]

@kesselb
Following #509 (comment) the actual issue seems that the recursive directory iterator does not work properly on FreeBSD.

I created a ticket on my hoster so they can investigate this issue. Usually they are quite quick and supportive.

Given that we don't list FreeBSD as supported operating system https://docs.nextcloud.com/server/latest/admin_manual/installation/system_requirements.html I suggest closing this pull request and the related issue.

Please do not do this!
Keep in mind that this PR actually tackles several issues:

• The first one ist the wrong usage of rmdir() where recursiveDelete() should be used. This is an obvious bug and should be fixed anyway if not for the bug then for the best practice!
• There where missing checks for the return values
• I also fixed several formating oddities.
• hostpoint.ch is the biggest hoster in Switzerland, and I am very sure they are not the only ones using FreeBSD! Beside of this issue, Nextcloud runs without any issues!

A user can always use the manual update.

Which a real pain since all apps have to be installed again separately!

@Altahrim
I am wondering if we could preload the file list. Something like:
[..]
Does it fix the issue for you? I think it will be faster if we avoid disk accesses.

Yes, this works as well. I updated the PR accordingly.

Under which OS did you test it?
I could only reproduce it with a FreeBSD 12.4.

[kesselb]

The first one ist the wrong usage of rmdir() where recursiveDelete() should be used. This is an obvious bug and should be fixed anyway if not for the bug then for the best practice!

updater/lib/Updater.php

Lines 285 to 298 in 55eca48

	/**
	* Gets the recursive directory iterator over the Nextcloud folder
	*
	* @return \RecursiveIteratorIterator<\RecursiveDirectoryIterator>
	*/
	private function getRecursiveDirectoryIterator(?string $folder = null): \RecursiveIteratorIterator {
	if ($folder === null) {
	$folder = $this->baseDir . '/../';
	}
	return new \RecursiveIteratorIterator(
	new \RecursiveDirectoryIterator($folder, \RecursiveDirectoryIterator::SKIP_DOTS),
	\RecursiveIteratorIterator::CHILD_FIRST
	);
	}

As explained before, the iterator will return the files before the directory and therefore rmdir will work fine.
That's not an obvious bug but something broken on your end as you stated yourself #509 (comment).

Which a real pain since all apps have to be installed again separately!

Upgrade guide: https://docs.nextcloud.com/server/latest/admin_manual/maintenance/manual_upgrade.html

Configure a custom app directory to make the upgrade easier: https://docs.nextcloud.com/server/latest/admin_manual/apps_management.html#using-custom-app-directories

[caco3]

Dear all

Thanks for your valuable comments.

I modified the PR as following:

• reverted my usages of recursiveDelete() back to use rmdir() as was before.
• replaced iterators with filelist where needed.
• added checks for rmdir() and unlink() where they where missing.

This way I was successfully able to update my Nextcloud.
When you look on the changes, this condenses now to a few minor changes:

• modified the iterators with first building the file/folder list and then loop through those lists. This only is needed where we actually modify the folders in the loops, namely when using rmdir, unlink() and rename(). As @Altahrim stated, this might use minimally more memory.
• fixed some log messages and log message formates.

[caco3]

Looking on all usage of in Nextcloud (https://github.com/search?q=org%3Anextcloud+RecursiveIteratorIterator%3A%3ACHILD_FIRST&type=code), there are some other places, where the RecursiveDirectoryIterators should be replaced by a filelist.

Note worthy also is following comment:
https://github.com/nextcloud/server/blob/b4e707059def28bf4544ae43272bf62d5662961b/lib/private/Files/Storage/Local.php#L132

So it seems I am not the first one who had issues with the RecursiveDirectoryIterator. In common also the usage of NFS as @joshtrichards found out.

[joshtrichards]

Looks like that's originally from a nearly 10 year ago OC issue here: owncloud/core#8376

[caco3]

@Altahrim What am I supposed to do for the failing "same code base" check?
It looks like it is expected that I also generate an updated PHAR file. But there is no documentation how to do this. Also I think this clearly is a job which should be done by a Github action (and the PHAR file should not be part of the repo either).

[blizzz]

@Altahrim What am I supposed to do for the failing "same code base" check? It looks like it is expected that I also generate an updated PHAR file. But there is no documentation how to do this. Also I think this clearly is a job which should be done by a Github action (and the PHAR file should not be part of the repo either).

commit results from make updater.phar.

[caco3]

But is make run by the pipeline or do I have to run it myself, locally?
In case it is run by the pipeline, why does it not do what it should?

In case I have to run it locally: I can't due to missing dependencies.

[blizzz]

I like the changes in the current form as this strategy is reasonable, simple and indeed safer to get the list of files first.

An issue with the memory footprint could theoretically be the data directory. While it should not lie within the nextcloud root, it might be in reality, and we don't control the size or contents.

The are some slight changes in early exceptions, but it does not change anything in general. ✔️

Especially because the updater is a sensitive part and could leave users stranded if there are issues, a lack of surprises is being fancied ;)

[blizzz]

But is make run by the pipeline or do I have to run it myself, locally? In case it is run by the pipeline, why does it not do what it should?

In case I have to run it locally: I can't due to missing dependencies.

The workflow is currently only a checker, it does not commit and push its results.

[caco3]

Thanks for the review.

I was now able to build the phar file and check it in. But there is now a conflict I am not privileged to solve.

There are also a bunch of vendor files modified during the make.
I don't think they should be commited, right?

[kesselb]

The changes to vendor/composer/installed.php look okay.

I cannot reproduce the other changes with your branch and Composer version 2.6.5 2023-10-06 10:11:52.
Are you using an older version of composer, perhaps?

Are you familiar with git enough to clean up the git history a bit?

. is not really a meaningful commit message. While we don't have strict rules on how to structure your commits, I would recommend grouping some things together. Like the code style changes and the actual bug fix. If someone is looking at the commit in a few years they will be happy about a good structure ;)

[caco3]

I have to dig into how to cleanup the history.
I usually just do a Squash when merging.

Can you first solve the conflict with the updater.phar?

[kesselb]

I usually just do a Squash when merging.

That's also fine.

Can you first solve the conflict with the updater.phar?

They were changes in master in the meantime.
I guess you have to update/sync your fork, then merge master into the branch, run make and commit the result. I don't have write permission for your fork and therefore cannot do it.

[kesselb]

As backup, the latest diff: 510.diff.txt

[caco3]

sorry for the mess 😒

I made a new PR with a clean history -> #515

[caco3]

I cannot reproduce the other changes with your branch and Composer version 2.6.5 2023-10-06 10:11:52.
Are you using an older version of composer, perhaps?

Yes, my Ubuntu still had an older version :(

[caco3]

An issue with the memory footprint could theoretically be the data directory. While it should not lie within the nextcloud root, it might be in reality, and we don't control the size or contents.

This should no longer be an issue after #507

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!