config: fix correctness issues in reading

[pulsejet]

Multiple fixes here.

1. Checking readability is pointless, since fopen is guaranteed to not throw and return false if the file doesn't exist or is not readable (it even logs a warning when the file isn't readable, which is very nice)
2. Skip even for non-main fails when we can't open it (it'll crash later anyway otherwise) Show the admin a better error message when a config file cannot be opened instead of throwing a TypeError (see config: fix correctness issues in reading #44230 (comment))
3. Reduce the size of the critical block
4. Make sure to unlock the file even if something fails
5. Check opcache on every request. This is low overhead since it only revalidates the timestamp for config.php (even if timestamp validation is disabled in the opcache config) and actually fixes a lot of correctness issues. Some actions such as upgrading an app make it obvious that the opcache may need to be refreshed, but config changes might happen more subtly, and opcache defeats the whole point of locking the file to begin with.

Side note: while Config::writeData does invalidate the opcache anyway, this is not sufficient since

1. It may be called from the CLI
2. It only affects a single PHP-FPM instance; other hosts wont be affected

[pulsejet]

I believe this is low risk and can be backported as a bugfix (there are technically two "bugs": the handle never being unlocked / closed in certain cases, and all the issues caused by the missing opcache invalidation)

[emoral435]

Overall, I like these changes a lot actually. No need to check if a file is readable or not if all we are doing is locking it :) just a small question regarding the sanitization that was removed for the configFilePath

EDIT: it does need to be removed after reading the explanation below!

[come-nc]

What bugs are you trying to fix? Are those bugs that you encountered yourself?

I’d expect PHP to release all locks on crash, I’m surprised the finally block is needed. Otherwise we’d need this kind of thing in a lot of places.
Also you are silencing the fopen errors, which will make understanding the situation harder in cases where fopen fails.

[pulsejet]

What bugs are you trying to fix? Are those bugs that you encountered yourself?

Opcache. For example pulsejet/memories#578 (comment)
I've lost count how many times something similar has been reported

I'm aware the server tuning docs mention the pitfalls of disabling revalidation, but revalidating one file (config.php) and every file on every request aren't the same thing (not even close). By revalidating just the config file for every request (which carries almost no cost), disabling general revalidation becomes much more realistic.

I’d expect PHP to release all locks on crash, I’m surprised the finally block is needed. Otherwise we’d need this kind of thing in a lot of places.

Correct, it's not strictly needed but this is cleaner / more resilient. In case the exception is caught, we no longer rely on PHP GC behavior the request to end for the file to unlock.

Also you are silencing the fopen errors, which will make understanding the situation harder in cases where fopen fails.

Errors like? There were checks on existence and readability so this would've never logged anything either way (this one is a performance fix to save a couple of kernel calls)

[pulsejet]

Also note, the locked/critical block should always be as small as possible (generally speaking), and the shortest way to do it is this.

[come-nc]

What bugs are you trying to fix? Are those bugs that you encountered yourself?

Opcache. For example pulsejet/memories#578 (comment) I've lost count how many times something similar has been reported

I'm aware the server tuning docs mention the pitfalls of disabling revalidation, but revalidating one file (config.php) and every file on every request aren't the same thing (not even close). By revalidating just the config file for every request (which carries almost no cost), disabling general revalidation becomes much more realistic.

We should not impact everyone to handle the case of a badly configured opcache. If opcache is used without revalidation it must be manually cleaned on app install/update, no? Or maybe it can be done automatically, don’t know. But disabling opcache for everyone on config file looks overkill.

I’d expect PHP to release all locks on crash, I’m surprised the finally block is needed. Otherwise we’d need this kind of thing in a lot of places.

Correct, it's not strictly needed but this is cleaner / more resilient. In case the exception is caught, we no longer rely on PHP GC behavior the request to end for the file to unlock.

Good point, did not think of the case it’s catched.

Also you are silencing the fopen errors, which will make understanding the situation harder in cases where fopen fails.

Errors like? There were checks on existence and readability so this would've never logged anything either way (this one is a performance fix to save a couple of kernel calls)

Any fopen failure other than non-existing file.
https://github.com/php/php-src/blob/master/main/fopen_wrappers.c#L700 too long path for instance.

[pulsejet]

We should not impact everyone to handle the case of a badly configured opcache. If opcache is used without revalidation it must be manually cleaned on app install/update, no? Or maybe it can be done automatically, don’t know.

I wouldn't say it's badly configured, this is actually good for performance (even the server tuning doc makes a passing mention). The problem isn't with app installation; opcache is reset when upgrading anyway. But the config file changing is what is the problem; this can happen more often and give rise to very subtle bugs.

But disabling opcache for everyone on config file looks overkill.

Opcache is not disabled, even on the config file. Just the timestamp is revalidated on every request, which is very minimal overhead (notice $force=false in the invalidate). Notably, checking the timestamp is actually much faster than opening and locking the file; things that we already do anyway. While the cost of this is negligible, the correctness guarantee it provides is pretty big.

Here's another way of looking at it: if we consider turning off revalidation as "bad", then the total cost paid here is basically nothing since the timestamp was validated anyway. On the other hand, just validating the config file makes turning off revalidation generally a "valid" opcache configuration (which we actually expect to work well), and reduce the overall overhead.

Any fopen failure other than non-existing file.

Point taken, though I wouldn't consider this a big deal. The check for a valid config file is needed only once (ever), but we pay the cost for every single request, which seems very excessive.

Anyway, can revert this one if you prefer.

[pulsejet]

More thoughts?

[come-nc]

Can you detail the rationale on "Skip even for non-main fails when we can't open it (it'll crash later anyway otherwise)" ?
With this PR the admin will have a hard time noticing the permissions on an additional config file are wrong, nothing is logged or shown.

[pulsejet]

Can you detail the rationale on "Skip even for non-main fails when we can't open it (it'll crash later anyway otherwise)" ? With this PR the admin will have a hard time noticing the permissions on an additional config file are wrong, nothing is logged or shown.

True ... the current behavior (before this patch) is odd; it just goes on with a false in the file and throws a TypeError. Looking at the error you can't get any idea what is wrong either.

I've updated the patch to show a better error. (note we can't actually throw here since the exception gets caught and the config not being loaded leads to more issues)

Before	After

Also updated PR description

[pulsejet]

I can't merge due to CI failure, seems unrelated

[emoral435]

Rerunning CI

[pulsejet]

/backport to stable28

[szaimen]

/backport to stable29

[backportbot]

The backport to stable29 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable29
git pull origin stable29

# Create the new backport branch
git checkout -b backport/44230/stable29

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick 15855876

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/44230/stable29

Error: No changes found in backport branch

---

Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

[pulsejet]

/backport to stable29

@szaimen this is already merged to 29

[szaimen]

Ah I see