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

Integrity constraint errors in oc_file_locks due to insufficient length of key column #25376

Closed
inty opened this Issue Jul 6, 2016 · 12 comments

Comments

Projects
None yet
3 participants
@inty

inty commented Jul 6, 2016

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 PVince81 added this to the 9.0.4 milestone Jul 6, 2016

@PVince81 PVince81 added the bug label Jul 6, 2016

@PVince81 PVince81 self-assigned this Jul 6, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 6, 2016

Member

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

Member

PVince81 commented Jul 6, 2016

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

@PVince81 PVince81 removed their assignment Jul 6, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 6, 2016

Member

@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.

Member

PVince81 commented Jul 6, 2016

@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 comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2016

Member

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.

Member

PVince81 commented Jul 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2016

Member

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.

Member

PVince81 commented Jul 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2016

Member

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 ?

Member

PVince81 commented Jul 7, 2016

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

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2016

Member

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 😄

Member

PVince81 commented Jul 7, 2016

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 😄

@guruz guruz modified the milestones: 8.2.7, 9.0.4 Jul 7, 2016

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz
Contributor

guruz commented Jul 7, 2016

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz

guruz Jul 7, 2016

Contributor

@icewind1991 turned it around in 9df60c7 ..
-> #25122 @PVince81 @nickvergessen

Contributor

guruz commented Jul 7, 2016

@icewind1991 turned it around in 9df60c7 ..
-> #25122 @PVince81 @nickvergessen

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz

guruz Jul 7, 2016

Contributor

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

Contributor

guruz commented Jul 7, 2016

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

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz

guruz Jul 7, 2016

Contributor

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

Contributor

guruz commented Jul 7, 2016

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

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 7, 2016

Member

@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)

Member

PVince81 commented Jul 7, 2016

@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)

guruz added a commit that referenced this issue Jul 8, 2016

@guruz guruz added the 2 - Developing label Jul 8, 2016

DeepDiver1975 added a commit that referenced this issue Jul 11, 2016

guruz added a commit that referenced this issue Jul 12, 2016

DeepDiver1975 added a commit that referenced this issue Jul 12, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jul 13, 2016

Member

PR was merged, closing

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

Member

PVince81 commented Jul 13, 2016

PR was merged, closing

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

@PVince81 PVince81 closed this Jul 13, 2016

guruz added a commit that referenced this issue Jul 13, 2016

PVince81 added a commit that referenced this issue Aug 8, 2016

Merge pull request #25472 from owncloud/92_testLongLock
Locking: Add testcases for lock length #25376
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment