Re: [Exist-development] Read lock problem in WebDAV causes database deadlock (Potentially Serious)

[Patrick B. <pat...@jo...>]

Update:

I may be reading this wrong, but does this else-case cause an unreleased
READ_LOCK on a collection?

-- webdav.methods.Lock.process() -- line 208:

            if(resource == null) {
                // No document found, maybe a collection
                LOG.info("resource==null, document not found.");
                collection = broker.openCollection(path,
org.exist.storage.lock.Lock.READ_LOCK);

                if(collection == null){
                    LOG.info("collection==null, path does not point to
collection");
                    LOG.debug("Create document as NullResource");
                    createNullResource(user, request, response, path);
                    isNullResource=true;


                } else {
                    String txt="Locking on collections not supported yet.";
                    LOG.debug(txt);

response.sendError(HttpServletResponse.SC_NOT_IMPLEMENTED, txt);
                    return;
                }


On Sun, Aug 7, 2011 at 9:22 PM, Patrick Bosek <pat...@jo...>wrote:

> Hi Everyone,
>
> After many hours of investigation I've found a locking situation in the
> WebDAV module which causes database deadlocks. The problem (as I see it) is
> that the webdav.methods.Lock.process() sets a READ_LOCK and has several
> cases where it exits without releasing it:
>
>
> -- webdav.methods.Lock.process() -- line 201:
>
> resource = broker.getXMLResource(path,
> org.exist.storage.lock.Lock.READ_LOCK);
>
> ....
>
> -- webdav.methods.Lock.process() -- line 249:* !!!!! As you can see in the
> case where the resource is already locked, it exists without releasing it's
> READ_LOCK*
>
>                 // Check if Resource is already locked.
>                 if( lock!=null && !lock.getName().equals(user.getName()) ){
>                     LOG.debug("Resource is locked.");
>                     response.sendError(SC_RESOURCE_IS_LOCKED,
>                             "Resource is locked by user "+ user.getName()
> +".");
>                     return;
>                 }
>
>                 // Check for request fo shared lock.
>                 if(lockToken.getScope() == LockToken.LOCK_SCOPE_SHARED) {
>                     LOG.debug("Shared locks are not implemented.");
>
> response.sendError(HttpServletResponse.SC_NOT_IMPLEMENTED,
>                             "Shared locks are not implemented.");
>                     return;
>                 }
>
>                 // Fill new Locktoken with new UUID
>                 lockToken.createOpaqueLockToken();
>                 resource.getMetadata().setLockToken(lockToken);
>                 resource.setUserLock(user);
>
>                 // Make token persistant
>                 TransactionManager transact = pool.getTransactionManager();
>                 Txn transaction = transact.beginTransaction();
>                 broker.storeXMLResource(transaction, resource);
>                 //TOUNDERSTAND : this lock is released below (in the
> finally clause)
>                 //isn't it rather a user lock release attempt ?
>                 // ?
>
> resource.getUpdateLock().release(org.exist.storage.lock.Lock.READ_LOCK); *!!!!!
> Here is where it is released when no one else has it locked*
>                 transact.commit(transaction);
>
>
>
> So, this is an issue which occurs in multi-user systems when two users try
> to access the same document.
>
> My proposed fix is to simply add releases to the two return cases. I don't
> really understand why the resource isn't just being unlocked in the finally
> in all cases, but I'm guessing there is a reason to release it before
> committing the transaction that I don't understand.
>
> My Fix:
>
>                 if( lock!=null && !lock.getName().equals(user.getName()) ){
>                     LOG.debug("Resource is locked.");
>                     response.sendError(SC_RESOURCE_IS_LOCKED,
>                             "Resource is locked by user "+ user.getName()
> +".");
>
>
> resource.getUpdateLock().release(org.exist.storage.lock.Lock.READ_LOCK);
>                     return;
>                 }
>
>                 // Check for request fo shared lock.
>                 if(lockToken.getScope() == LockToken.LOCK_SCOPE_SHARED) {
>                     LOG.debug("Shared locks are not implemented.");
>
> response.sendError(HttpServletResponse.SC_NOT_IMPLEMENTED,
>                             "Shared locks are not implemented.");
>
>
> resource.getUpdateLock().release(org.exist.storage.lock.Lock.READ_LOCK);
>                     return;
>                 }
>
>
> I can provide a patch or simply commit this into the 1.4.x branch if
> requested to do so by one of the core developers. But I wanted to ask first,
> since there are things going on here that are above my head.
>
> Thoughts?
>
>
> Cheers,
>
> --
> Patrick Bosek
> Jorsek Software
> Cell (585) 820 9634
> Office (877) 492 2960
> Jorsek.com
>
>


-- 
Patrick Bosek
Jorsek Software
Cell (585) 820 9634
Office (877) 492 2960
Jorsek.com