Repository navigation
[Bug]: oc_share table has duplicate rows for the same share #53970
Description
Activity
cc @nickvergessen @miaulalala @provokateurin to consider as part of planned performance improvements
Few notes:
- The query at is not hitting an index:
MariaDB [oc]> EXPLAIN SELECT id FROM oc_share WHERE share_type = 2 AND share_with = '…' AND parent = 42 AND item_type IN ('file', 'folder'); +------+-------------+----------+------+---------------------------------------------------------------------+--------------+---------+-------+------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+----------+------+---------------------------------------------------------------------+--------------+---------+-------+------+-------------+ | 1 | SIMPLE | oc_share | ref | item_share_type_index,share_with_index,parent_index,share_type_with | parent_index | 9 | const | 1 | Using where | +------+-------------+----------+------+---------------------------------------------------------------------+--------------+---------+-------+------+-------------+ 1 row in set (0.000 sec)
- The
formatShareAttributesbetween the read and write is creating more chances for concurrency, could be moved before the SELECT to reduce chance for concurrency - There is no lock/transaction around this
- I once had a draft to remove the item_type when we say we don't support oc_share for other share types: perf(sharing): Move item_type validation to PHP spreed#11548
But even without the query is still not using an index on prod
maybe related.
if you set your nextcloud to "allow users to set custom share link tokens" you can set a custom share link token in the sharing context of the object. BUT it isn't checked if the custom token ist already set for another object. a unique constraint could be enough here. though, i don't know about other dependencies .. so this may not be a correct solution .
and it would be nice if a real deletion of an object could trigger a deletion of the share entry too.
a unique constraint could be enough here.
That would only work, when all shares would have a token. But user, group and many other shares dont
@nickvergessen @Antreesy
I traced this and found the duplicate USERROOM rows originate from SharedMount::verifyMountPoint:server/apps/files_sharing/lib/SharedMount.php
Lines 104 to 107 in f3ca2a6
When a share provider sets a non-final target (e.g. a placeholder path that gets rewritten per-user via VerifyMountPointEvent), $newMountPoint !== $share->getTarget() evaluates true on every mount build, so updateFileTarget() is invoked → IManager::moveShare() → RoomShareProvider::move().
move() does a check-then-insert on oc_share without a unique constraint or ON CONFLICT handling. Under concurrent PROPFINDs (or PROPFIND + a parallel notification/sync poll triggered by SetupManager invalidating the mount cache on ShareCreatedEvent), both calls SELECT empty before either commits, then both INSERT — producing duplicate USERROOM rows with the same parent, share_with, file_target, and stime.
Reproducible by sharing a file into a Talk room (TYPE_ROOM share) and triggering two PROPFINDs on the recipient's mount near-simultaneously.
Possible fixes: unique index on (share_type, parent, share_with), or INSERT … ON CONFLICT DO NOTHING in move() / deleteFromSelf().
I can send a PR for possible fixes.
Reacted by Michael Pardatscher and mostafa khaki
Bug description
There might be a race condition in ShareProvider, where several requests in parallel check if share exists in DB, and create it otherwise. In this case, there could be several new entries created, each with unique id, but all pointing to the same share.
If user tries to modify/leave share, it will modify the first row only, keeping the second intact. That way, you can never get rid of it.
Steps to reproduce
Don't have clear steps, but example behaviour sounds logical:
share_type10 andfile_target/{TALK_PLACEHOLDER}/file.mdshare_type11 andfile_target/Talk/file.md (best reproducible when PHP debugger was enabled, but also occurs on prod/daily instances)3.1. Worse case if user at some point changed the attachments folder, so initial entry is
share_type11 andfile_target/Talk/file.md, and duplicates areshare_type11 andfile_target/SomeOtherPath/file.md. Modifications will touch only first occurence of /SomeOtherPath/file.mdExpected behavior
Some sort of transactional lock (to write in DB only once) in place
Nextcloud Server version
master
Details
Operating system
None
PHP engine version
None
Web server
None
Database engine version
None
Is this bug present after an update or on a fresh install?
None
Are you using the Nextcloud Server Encryption module?
None
What user-backends are you using?
Configuration report
List of activated Apps
Nextcloud Signing status
Nextcloud Logs
Additional info
No response