Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stable28] fix: don't return null for SharedStorage::getWrapperStorage with share recursion #44323

Merged
merged 2 commits into from
Jul 11, 2024

Conversation

backportbot[bot]
Copy link

@backportbot backportbot bot commented Mar 19, 2024

Backport of #44132

Warning, This backport's changes differ from the original and might be incomplete ⚠️

Todo

  • Review and resolve any conflicts
  • Amend HEAD commit to remove the line stating to skip CI

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

@backportbot backportbot bot added bug 3. to review Waiting for reviews labels Mar 19, 2024
@backportbot backportbot bot added this to the Nextcloud 28.0.4 milestone Mar 19, 2024
@icewind1991 icewind1991 force-pushed the backport/44132/stable28 branch from ec1c6f9 to 05d54cc Compare March 19, 2024 14:10
@icewind1991 icewind1991 marked this pull request as ready for review March 19, 2024 14:10
@come-nc
Copy link
Contributor

come-nc commented Mar 19, 2024

@icewind1991 Have you managed to run into this code path? I do not understand in which case can initialized be false and storage null at the same time.

@Altahrim Altahrim mentioned this pull request Mar 21, 2024
9 tasks
@icewind1991
Copy link
Member

@icewind1991 Have you managed to run into this code path? I do not understand in which case can initialized be false and storage null at the same time.

The process of getting the share source could lead to it recursing into the function if there (somehow) is a recursive share.

This shouldn't happen but when it does this PR improves the error behavior

@come-nc
Copy link
Contributor

come-nc commented Mar 25, 2024

Oh, thank you, I think I get it now.

My first thought was moving $this->initialized = true to the end but I think it would make the recursion infinite loop actually…

Comment on lines +145 to +157
if (!$this->storage) {
// marked as initialized but no storage set
// this is probably because some code path has caused recursion during the share setup
// we setup a "failed storage" so `getWrapperStorage` doesn't return null.
// If the share setup completes after this the "failed storage" will be overwritten by the correct one
$this->logger->warning('Possible share setup recursion detected');
$this->storage = new FailedStorage(['exception' => new \Exception('Possible share setup recursion detected')]);
$this->cache = new FailedCache();
$this->rootPath = '';
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!$this->storage) {
// marked as initialized but no storage set
// this is probably because some code path has caused recursion during the share setup
// we setup a "failed storage" so `getWrapperStorage` doesn't return null.
// If the share setup completes after this the "failed storage" will be overwritten by the correct one
$this->logger->warning('Possible share setup recursion detected');
$this->storage = new FailedStorage(['exception' => new \Exception('Possible share setup recursion detected')]);
$this->cache = new FailedCache();
$this->rootPath = '';
}
if (!$this->storage) {
// marked as initialized but no storage set
// this is probably because some code path has caused recursion during the share setup
// we setup a "failed storage" so `getWrapperStorage` doesn't return null.
// If the share setup completes after this the "failed storage" will be overwritten by the correct one
throw new \Exception('Possible share setup recursion detected')]);
}

This should work then, and it would be caught by the try/catch below?
Not sure if it’s cleaner because it would mean the function is now throwing.

@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
@Altahrim Altahrim mentioned this pull request Apr 17, 2024
5 tasks
@Altahrim
Copy link
Collaborator

There is a conflict on this one

@Altahrim Altahrim mentioned this pull request Apr 24, 2024
This was referenced May 15, 2024
@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Jun 7, 2024
@blizzz blizzz mentioned this pull request Jun 11, 2024
6 tasks
@blizzz blizzz mentioned this pull request Jun 17, 2024
9 tasks
@blizzz blizzz mentioned this pull request Jun 25, 2024
@Altahrim Altahrim mentioned this pull request Jul 10, 2024
…e recursion

Signed-off-by: Robin Appelman <robin@icewind.nl>
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 force-pushed the backport/44132/stable28 branch from 05d54cc to e0f3de8 Compare July 11, 2024 15:40
@AndyScherzinger AndyScherzinger merged commit 7b2c183 into stable28 Jul 11, 2024
54 checks passed
@AndyScherzinger AndyScherzinger deleted the backport/44132/stable28 branch July 11, 2024 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants