Bug 52559

Summary: [PATCH] Some PROPPATCH causing segfault in mod_dav_fs / mod_dav
Product: Apache httpd-2 Reporter: Diego Santa Cruz <Diego.SantaCruz>
Component: mod_dav_fsAssignee: Apache HTTPD Bugs Mailing List <bugs>
Severity: normal CC: wiml
Priority: P2 Keywords: PatchAvailable
Version: 2.2.21   
Target Milestone: ---   
Hardware: All   
OS: All   
Attachments: First patch to fix PROPPATCH segfault
Do not fail PROPPATCH when prop namespace not known
Do not segfault on PROPFIND with a zero length DBM

Description Diego Santa Cruz 2012-01-30 11:24:35 UTC
With our product we stumbled across a segfault in mod_dav_fs / mod_dav, as follows.

When a PROPPATCH attempts to remove a non-existent dead property on a resource for which there is no dead property in the same namespace httpd segfaults (e.g., attempt to remove a property 'foo' in namespace 'http://example.com/bar' on a new resource which has no other dead properties).

Furthermore, if the resource had no dead properties, after this segfault httpd leaves the dead property DBM in a state which causes an httpd segfault on every PROPFIND (the .pag and .dir files are both zero length).

From our analysis the problem boils down to three issues. A patch for each follows.
Comment 1 Diego Santa Cruz 2012-01-30 11:31:18 UTC
Created attachment 28228 [details]
First patch to fix PROPPATCH segfault

This patch fixes the described segfault in the PROPPATCH.

In the described case a rollback cannot be created when doing the dav_method_proppacth(), because dav_propdb_get_rollback() fails without returning a rollback object. Thus the call to dav_propdb_apply_rollback() segfaults because rollback is NULL. This patch just protects against that case considering it a success.
Comment 2 Diego Santa Cruz 2012-01-30 12:31:23 UTC
Created attachment 28229 [details]
Do not fail PROPPATCH when prop namespace not known

With the patch in attachment 28228 [details] the PROPPATCH described above no longer segfaults httpd, but fails with a 500 error.

As stated in RFC 4918 (WebDAV), section 14.23, removing a non-existing property is not an error.

The failure comes from dav_propdb_get_rollback(), which returns an error for the non-existent property. It turns out that if the property's namespace is not known (as can well be the case for a non-existent property) dav_build_key() returns a zeroed key, but doing a dav_dbm_fetch() on such a key returns an error. In fact, APR's apr_dbm_fetch() returns APR_EINVAL for such a key (at least the SDBM backend).

The patch modifies dav_dbm_fetch() so that is catches this case and treats it as a "key not found", which is the actual error as the namespace index is built from the DBM.

With this patch httpd succeds the test case described above (doing a PROPPATCH to remove a non-existing property on a resource which does not have any other property in the same namespace).
Comment 3 Diego Santa Cruz 2012-01-30 12:43:07 UTC
Created attachment 28230 [details]
Do not segfault on PROPFIND with a zero length DBM

As described above, when httpd segfaults during the PROPPATCH it leaves a zero length DBM if no other dead properties existed for the resource. Doing a PROPFIND on the resource segfaults httpd.

The cause of the segfault is that dav_get_allprops() does not check the return value of the first_name() nor next_name() DB hooks for errors. When the DBM is of zero length (both the .dir and .pag files are zero length) first_name() returns an error and leaves its 'name' argument uninitialized. But then 'name.ns' is accessed just after the first_name() call, possibly causing a segfault or other errors as 'name' is stack allocated.

The attached patch changes this so that the return value of first_name() and next_name() is checked and the while loop on the properties be stopped in case of error.

As it seems that dav_get_allprops() cannot return an error I could not see another way to handle this situation and this is how errors on the output_value() hook call are treated within dav_get_allprops() anyhow.
Comment 4 Diego Santa Cruz 2012-01-30 12:44:37 UTC
Forgot to mention, but the tests have been done against httpd 2.2.21. The attached patches are against this version too.
Comment 5 Wim Lewis 2013-04-17 18:57:10 UTC
I can confirm this crash still occurs in Apache 2.4.4: the PROPFIND causes a segfault and leaves the zero-length DAV prop db behind. (However, the zero-length prop db no longer causes later operations to crash the server, so that's progress I guess.)

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000010
0x0000000100602c53 in dav_propdb_apply_rollback ()
(gdb) bt
#0  0x0000000100602c53 in dav_propdb_apply_rollback ()
#1  0x00000001000c8d2d in dav_prop_rollback ()
#2  0x00000001000c1b0e in dav_process_ctx_list ()
#3  0x00000001000c2074 in dav_method_proppatch ()
#4  0x00000001000c7063 in dav_handler ()
#5  0x00000001000015ef in ap_run_handler ()
#6  0x0000000100001eaf in ap_invoke_handler ()
#7  0x000000010005b7de in ap_process_async_request ()
#8  0x000000010005b8b0 in ap_process_request ()
#9  0x00000001000573fb in ap_process_http_sync_connection ()
#10 0x00000001000574f6 in ap_process_http_connection ()
#11 0x0000000100019906 in ap_run_process_connection ()
#12 0x0000000100019dd7 in ap_process_connection ()
#13 0x00000001000e24d8 in child_main ()
#14 0x00000001000e25e4 in make_child ()
#15 0x00000001000e2c5d in prefork_run ()
#16 0x000000010001c47d in ap_run_mpm ()
#17 0x000000010000d924 in main ()
Comment 6 Wim Lewis 2013-04-22 22:22:43 UTC
The crash also still occurs in trunk/2.5 r1470659, no apparent change from 2.4.4
Comment 7 Graham Leggett 2013-04-27 17:39:13 UTC
First patch applied to trunk in r1476642.
Comment 8 Graham Leggett 2013-04-27 17:51:15 UTC
Second patch applied to trunk in r1476644.
Comment 9 Graham Leggett 2013-04-27 17:52:37 UTC
Third patch applied to trunk in r1476645.
Comment 10 Graham Leggett 2013-04-27 19:07:54 UTC
Can you test and verify this works for you?

Backport proposed for v2.4.
Comment 11 Graham Leggett 2013-05-12 10:30:31 UTC
Backported to v2.4.5.
Comment 12 Graham Leggett 2013-05-23 13:20:19 UTC
Proposed for backport to v2.2.
Comment 13 Wim Lewis 2015-03-11 23:52:16 UTC
Confirming I can no longer reproduce this in the 2.4.x series.