[Bug]: `oc_share` table has duplicate rows for the same share

[Antreesy]

⚠️ This issue respects the following points: ⚠️

• This is a bug, not a question or a configuration/webserver/proxy issue.
• This issue is not already reported on Github OR Nextcloud Community Forum (I've searched it).
• Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
• I agree to follow Nextcloud's Code of Conduct.

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:

1. Sharing a file in Talk creates a parent entry with share_type 10 and file_target /{TALK_PLACEHOLDER}/file.md
2. User access conversation with shared file as last message, filesystem is mounted from conversations request and chat messages request
3. This creates two entries in DB with share_type 11 and file_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_type 11 and file_target /Talk/file.md, and duplicates are share_type 11 and file_target /SomeOtherPath/file.md. Modifications will touch only first occurence of /SomeOtherPath/file.md

Expected 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?

• Default user-backend (database)
• LDAP/ Active Directory
• SSO - SAML
• Other

Configuration report

List of activated Apps

Nextcloud Signing status

Nextcloud Logs

Additional info

No response

[Antreesy]

cc @nickvergessen @miaulalala @provokateurin to consider as part of planned performance improvements

[nickvergessen]

Few notes:

1. The query at

   server/lib/private/Share20/DefaultShareProvider.php

   Line 561 in ae16a28

   ->andWhere($qb->expr()->in('item_type', $qb->createNamedParameter(['file', 'folder'], IQueryBuilder::PARAM_STR_ARRAY)))

   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)

2. The formatShareAttributes between the read and write is creating more chances for concurrency, could be moved before the SELECT to reduce chance for concurrency
3. There is no lock/transaction around this

[nickvergessen]

• 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