Core: Integrity constraint errors in oc_file_locks due to insufficient length of key column

[inty]

Steps to reproduce

I can't seem to reproduce this myself. Either because it only happens with previously created shares or it's related to the Windows client, I don't know. But the issue is pretty clear, we get this error for certain folders from the Windows client and the web interface.

Integrity constraint violation: 1062 Duplicate entry 'scanner::shared::[OMITTED]' for key 'lock_key_index'

The problem here seems to be that for these folders it doesn't use a md5 for the key for some reason and the key column of oc_file_locks is only 64 chars so anything longer than that (minus the prefix obviously) will generate constraint errors.

I fixed this for now by changing the key column to varchar(255), seems to be working.

First I thought this might have been a bug with strlen and our usage of "åäö" in paths but that doesn't seem to be it. I can't reproduce it when I create a new share anyhow.

Expected behaviour

Locking should not produce these errors for certain paths over certain length.

Actual behaviour

Syncing fails.

Server configuration

Operating system:
Debian Wheezy
Web server:
Apache 2.2
Database:
MySQL 5.5
PHP version:
5.4
ownCloud version: (see ownCloud admin page)
8.2.6
Updated from an older ownCloud or fresh install:
Updated from older
Where did you install ownCloud from:
Your Open Build Service
List of activated apps:

If you have access to your command line run e.g.:
sudo -u www-data php occ app:list
from within your ownCloud installation folder

The content of config/config.php:

{
    "system": {
        "instanceid": "oc836321d4d1",
        "passwordsalt": "***REMOVED SENSITIVE VALUE***",
        "trusted_domains": [
            "owncloud.chas.se"
        ],
        "datadirectory": "\/var\/www\/owncloud\/data",
        "overwrite.cli.url": "https:\/\/owncloud.xxx.se",
        "dbtype": "mysql",
        "version": "8.2.6.2",
        "dbname": "owncloud",
        "dbhost": "localhost",
        "dbtableprefix": "oc_",
        "dbuser": "***REMOVED SENSITIVE VALUE***",
        "dbpassword": "***REMOVED SENSITIVE VALUE***",
        "installed": true,
        "mail_smtpmode": "smtp",
        "ldapIgnoreNamingRules": false,
        "forcessl": true,
        "mail_smtpsecure": "ssl",
        "mail_smtpauthtype": "PLAIN",
        "mail_smtpauth": 1,
        "mail_smtphost": "mail.chas.se",
        "mail_smtpport": "465",
        "mail_smtpname": "***REMOVED SENSITIVE VALUE***",
        "mail_smtppassword": "***REMOVED SENSITIVE VALUE***",
        "mail_domain": "chas.se",
        "mail_from_address": "owncloud",
        "theme": "",
        "maintenance": false,
        "secret": "***REMOVED SENSITIVE VALUE***",
        "memcache.local": "\\OC\\Memcache\\Memcached",
        "memcached_servers": [
            [
                "localhost",
                11211
            ]
        ],
        "loglevel": 2,
        "trashbin_retention_obligation": "auto"
    }
}

Are you using external storage, if yes which one: local/smb/sftp/...
NFS
Are you using encryption: yes/no
Yes

[PVince81]

Hmmm, then it's a bug. It should use md5 there.

[PVince81]

@guruz do you have time to have a look ?
Should be a matter of using the md5 of the file in the piece that creates the "scanner::" key.
Just use the md5 function.

[PVince81]

This is weird, because from what I can see the call to $storage->acquireLock('scanner::', ...) should reach the code in the Common storage which already does a md5: https://github.com/owncloud/core/blob/v9.0.3/lib/private/files/storage/common.php#L663

Unless it's some exotic storage implementation that implements its own acquireLock in a different way ? AFAIK all storages extend from Common.

[PVince81]

The error message Duplicate entry 'scanner::shared::[OMITTED]' for key 'lock_key_index' seem to imply that it tried to insert the value directly without doing a md5 on it.

So it seems there is a code path that bypasses it. It could be the SharedStorage.

But SharedStorage is not supposed to create locks at all. It must refer to the source storage instead.

[PVince81]

Furthermore, to create the string "scanner::shared::[OMITTED]" it means that the value of $path was "shared::[OMITTED]" AND also bypass the md5. Weird stuff.

SharedStorage seems to properly refer to the underlying source storage: https://github.com/owncloud/core/blob/v9.0.3/apps/files_sharing/lib/sharedstorage.php#L685
And the federated share storage class extends from DAV which extends from Common.

@inty are you using a custom Storage implementation ? Or did you at some point remove the md5 call here https://github.com/owncloud/core/blob/v9.0.3/lib/private/files/storage/common.php#L663 to debug locks and forgot to add it back ?

[PVince81]

AHA, look at this:

In 8.2.6 it's going directly to the locking provider, bypassing the md5: https://github.com/owncloud/core/blob/v8.2.6/lib/private/files/cache/scanner.php#L264
But on 9.0.3 (which I was looking at all the time) it's fixed to use the other way: https://github.com/owncloud/core/blob/v9.0.3/lib/private/files/cache/scanner.php#L299

So we need to find the PR that fixed it in v9.0.x and backport it to 8.2.6.
And also add more tests as @guruz is doing now :smile:

[guruz]

It seems to be fixed later in 8.2.x? https://github.com/owncloud/core/pull/24260/files but then it is not in stable8.2.. heh.. https://github.com/owncloud/core/blob/stable8.2/lib/private/files/cache/scanner.php#L264

[guruz]

@icewind1991 turned it around in https://github.com/owncloud/core/commit/9df60c7956f5efa856fcadb06e879288f08e065b ..
-> https://github.com/owncloud/core/pull/25122 @PVince81 @nickvergessen

[guruz]

Was this done because of $targetInternalPath and resolvePath? https://github.com/owncloud/core/blob/stable8.2/apps/files_sharing/lib/sharedstorage.php#L655

[guruz]

One fix would be to do the md5ing directly in public function scan

[PVince81]

@icewind1991 turned it around in 9df60c7 ..

Argh, yeah, then add the md5 call in the row that does lockingWrapper->acquireLock.

I seem to remember @icewind1991 had to fix it in 8.2 because there was a bug specific in that version that disappeared later in 9.0... Might also need to check if 8.1 is affected. (it had locking too, an earlier version)

[PVince81]

PR was merged, closing

@guruz can you port the unit test to master/stable9* ?

[lock[bot]]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.