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

[Adam R. <ad...@ex...>]

I have just studied the code, and if I understand it correctly then
there are a couple of things that seem incorrect here. It early and I
cant sleep.

Dannes is the master of all things WebDAV, so he may be able to show
us what we are misunderstanding.

Your comment about the read-lock -- it seems to me that the finally
resource lock release, only happens if isNullResource is set, yet when
isNullResource is set, the resource is never read locked anyway as far
as I can tell.

IMHO, this function could be improved by being split into two, its too
long and it takes two different actions, in the least it could call
out to a function to handle the null resource path. This would make
the code easier to understand and test.

In its current state I think line 277 should be deleted, and also line
294, and this should fix the locking issue.

But in addition it seems to me, that this function reports to WebDAV
that resources have been locked, but this is not the case as it always
unlocks the resource. I am not sure why this would be, but I would of
imagined that the resource should remain locked until WebDAV unlocks
it?



On 8 August 2011 03:22, 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
>
>
> ------------------------------------------------------------------------------
> BlackBerry&reg; DevCon Americas, Oct. 18-20, San Francisco, CA
> The must-attend event for mobile developers. Connect with experts.
> Get tools for creating Super Apps. See the latest technologies.
> Sessions, hands-on labs, demos & much more. Register early & save!
> http://p.sf.net/sfu/rim-blackberry-1
> _______________________________________________
> Exist-development mailing list
> Exi...@li...
> https://lists.sourceforge.net/lists/listinfo/exist-development
>
>



-- 
Adam Retter

eXist Developer
{ United Kingdom }
ad...@ex...
irc://irc.freenode.net/existdb