Bug 51297 - Improve error handling during "UNLOCK"
Summary: Improve error handling during "UNLOCK"
Status: NEW
Alias: None
Product: Apache httpd-2
Classification: Unclassified
Component: mod_dav (show other bugs)
Version: 2.5-HEAD
Hardware: All Linux
: P2 normal with 12 votes (vote)
Target Milestone: ---
Assignee: Apache HTTPD Bugs Mailing List
Keywords: PatchAvailable
Depends on:
Reported: 2011-05-31 07:23 UTC by vijayaguru
Modified: 2011-05-31 09:08 UTC (History)
1 user (show)

patch that improves error handling during "dav unlock" (6.09 KB, patch)
2011-05-31 09:06 UTC, vijayaguru
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description vijayaguru 2011-05-31 07:23:56 UTC
While unlocking a file forcibly using subversion client, and if the unlock is blocked by pre-unlock hook script, the client is getting "500 Internal Server Error" instead of the error message sent by the subversion server.

Simple reproduction recipe:

1. Lock a file. "svn lock testfile.txt --username user1"

2. Make pre-unlock hook script to fail.

3. Try to unlock the file forcibly as a different user. "svn unlock testfile.txt --force --username user2"

step 3 fails with "500 Internal Server Error" message.

Whenever the pre-unlock hook fails, Subversion server sends the error message "Unlock blocked by pre-unlock hook (exit 
code 1)...." to mod_dav. But mod_dav is not handling the error and it is not sending any error messages to 
client side, instead it sends only HTTP_STATUS code alone(confirmed it with the following codepath). 

From dav/main/mod_dav.c: dav_method_unlock(),

    if ((result = dav_unlock(r, resource, locktoken)) != OK) {
        return result;

From dav/main/util_lock.c: dav_unlock(),

    err = (*repos_hooks->walk)(&ctx.w, DAV_INFINITY, &multi_status);

    /* ### fix this! */
    /* ### do something with multi_status */
    result = err == NULL ? OK : err->status;


    return result;


This walk() method finds out the resource to unlock and walker callback functin dav_unlock_walker() removes lock from that resource.

From dav/main/util_lock.c: dav_unlock_walker(),

    if ((err = (*ctx->w.lockdb->hooks->remove_lock)(ctx->w.lockdb,
                                                    ctx->locktoken)) != NULL) {
        /* ### should we stop or return a multistatus? looks like STOP */
        /* ### add a higher-level description? */
        return err;


In this case, unlock is blocked by pre-unlock hook, subvesion server marshalls the error back to mod_dav, mod_dav's dav_unlock_walker() returns the error sent by svn server, but dav_unlock() returns only the HTTP_STATUS code(err->status) and the error message is *lost* there.
Comment 1 vijayaguru 2011-05-31 09:06:29 UTC
Created attachment 27095 [details]
patch that improves error handling during "dav unlock"

Attaching the patch that fixes this issue.

This patch does the following things to maintain backward compatibility.

1. Copied dav/main/util_lock.c: dav_unlock() to a static function do_dav_unlock() which accepts "dav_error **err" as an outparam and preserves any error in case of failures.

2. dav_unlock() will call do_dav_unlock() and return HTTP_STATUS code in case of any failures; Otherwise return OK.(Old callers can still use this method)

3.dav_unlock2() will also call do_dav_unlock() and return dav_error type in case of any failures; Otherwise return NULL.

4.dav/main/mod_dav.c: dav_method_unlock() will call dav_unlock2() and *handles* the error in case of any failures.
Comment 2 vijayaguru 2011-05-31 09:08:51 UTC
Updating Keyword as "PatchAvailable".