Re: lock_tests-9 is segfaulting over ra_dav

Philip Martin <philip@codematters.co.uk> writes:

> Jani Averbach <jaa@jaa.iki.fi> writes:
>
>> The lock-test 9 is segfaulting over ra_dav with trunk@14272 (at
>> least). I will slightly modify svntest so I can tell for sure when and
>> with what it is segfaulting after next run of the svntest.
>>
>> svntest $ ~/inst/svn_trunk/bin/svn unlock \
>> --username=jrandom --password=rayjandom \
>> http://localhost:42024/repositories/lock_tests-9/iota
>> Segmentation fault
>
> The first valgrind error is
>
> $ valgrind -q svn "unlock" "--username" "jrandom" "--password" "rayjandom" "http://localhost:8888/repositories/lock_tests-9/iota"
> ==25529== Invalid read of size 4
> ==25529== at 0x1BA1B4C2: create_request_hook (session.c:896)
> ==25529== by 0x1BBF5AA3: ne_request_create (in /usr/lib/libneon.so.24.0.7)
> ==25529== by 0x1BC00C64: ne_unlock (in /usr/lib/libneon.so.24.0.7)
> ==25529== by 0x1BA1BE82: shim_svn_ra_dav__unlock (session.c:1176)
> ==25529== by 0x1BA1C059: svn_ra_dav__unlock (session.c:1241)
> ==25529== by 0x1B96707F: svn_ra_unlock (ra_loader.c:525)
> ==25529== by 0x1B922259: svn_client_unlock (locking_commands.c:422)
> ==25529== by 0x8057BDA: svn_cl__unlock (unlock-cmd.c:56)
> ==25529== by 0x80524FD: main (main.c:1442)
> ==25529== Address 0x1BF77654 is 28 bytes inside a block of size 32 free'd
> ==25529== at 0x1B901AA8: free (vg_replace_malloc.c:152)
> ==25529== by 0x1BB7AEDE: pool_clear_debug (apr_pools.c:1376)
> ==25529== by 0x1BB7B036: apr_pool_destroy_debug (apr_pools.c:1437)
> ==25529== by 0x1B921EC4: fetch_tokens (locking_commands.c:318)
> ==25529== by 0x1B922205: svn_client_unlock (locking_commands.c:415)
> ==25529== by 0x8057BDA: svn_cl__unlock (unlock-cmd.c:56)
> ==25529== by 0x80524FD: main (main.c:1442)

Looking at libsvn_ra_dav/session.c, the functions
shim_svn_ra_dav__lock and shim_svn_ra_dav__unlock both contain the
code:

  /* Clear out the lrb... */
  memset((ras->lrb), 0, sizeof(ras->lrb));

Huh? That ras->lrb is a pointer so that makes little sense, probably
it should be sizeof(*ras->lrb).

The next problem is the lifetime of the neon request hooks. The
function setup_neon_request_hook contains the comment:

  /* We need to set up the lock callback once and only once per neon
     session creation. */

but I see nothing that enforces that. The functions
svn_ra_dav__unlock and svn_ra_dav__lock both call
setup_neon_request_hook and may do so more than once, we need to check
that. Also, the pool passed to setup_neon_request_hook may have a
lifetime shorter than the session so the hooks may last longer than
the allocated memory, I think the session pool should be used.

Finally, the function svn_ra_dav__get_lock sets up a neon request hook
without calling setup_neon_request_hook, and it also uses a pool with
a lifetime shorter than the session, which I believe is causing the
valgrind error above. I think it should be using the
setup_neon_request_hook function.

Something like the following perhaps?

Fix a neon hook lifetime problem.

* subversion/libsvn_ra_dav/session.c
  (setup_neon_request_hook): Use the session pool instead of passing a
    pool argument, don't setup the hook more than once.
  (shim_svn_ra_dav__lock, shim_svn_ra_dav__unlock): memset the
    log_request_baton struct not the pointer.
  (svn_ra_dav__get_lock): Use setup_neon_request_hook to setup the
    hook rather than coding it directly.

Index: subversion/libsvn_ra_dav/session.c
===================================================================
--- subversion/libsvn_ra_dav/session.c (revision 14278)
+++ subversion/libsvn_ra_dav/session.c (working copy)
@@ -959,21 +959,23 @@
 
 
 static void
-setup_neon_request_hook(svn_ra_dav__session_t *ras,
- apr_pool_t *pool)
+setup_neon_request_hook(svn_ra_dav__session_t *ras)
 {
   /* We need to set up the lock callback once and only once per neon
      session creation. */
 
- struct lock_request_baton *lrb;
- /* Build context for neon callbacks and then register them. */
- lrb = apr_pcalloc(pool, sizeof(*lrb));
+ if (! ras->lrb)
+ {
+ struct lock_request_baton *lrb;
+ /* Build context for neon callbacks and then register them. */
+ lrb = apr_pcalloc(ras->pool, sizeof(*lrb));
 
- ne_hook_create_request(ras->sess, create_request_hook, lrb);
- ne_hook_pre_send(ras->sess, pre_send_hook, lrb);
+ ne_hook_create_request(ras->sess, create_request_hook, lrb);
+ ne_hook_pre_send(ras->sess, pre_send_hook, lrb);
 
- lrb->pool = pool;
- ras->lrb = lrb;
+ lrb->pool = ras->pool;
+ ras->lrb = lrb;
+ }
 }
 
 /* ### TODO for 1.3: Send all locks to the server at once. */
@@ -999,7 +1001,7 @@
                                         url, SVN_INVALID_REVNUM, pool));
 
   /* Clear out the lrb... */
- memset((ras->lrb), 0, sizeof(ras->lrb));
+ memset((ras->lrb), 0, sizeof(*ras->lrb));
 
   /* ...and load it up again. */
   ras->lrb->pool = pool;
@@ -1079,7 +1081,7 @@
   apr_hash_index_t *hi;
   apr_pool_t *iterpool = svn_pool_create (pool);
 
- setup_neon_request_hook(session->priv, pool);
+ setup_neon_request_hook(session->priv);
 
   /* ### TODO for 1.3: Send all the locks over the wire at once. This
      loop is just a temporary shim. */
@@ -1166,7 +1168,7 @@
     }
 
   /* Clear out the lrb... */
- memset((ras->lrb), 0, sizeof(ras->lrb));
+ memset((ras->lrb), 0, sizeof(*ras->lrb));
 
   /* ...and load it up again. */
   ras->lrb->pool = pool;
@@ -1215,7 +1217,7 @@
   apr_hash_index_t *hi;
   apr_pool_t *iterpool = svn_pool_create (pool);
 
- setup_neon_request_hook(session->priv, pool);
+ setup_neon_request_hook(session->priv);
 
   /* ### TODO for 1.3: Send all the lock tokens over the wire at once.
         This loop is just a temporary shim. */
@@ -1328,7 +1330,6 @@
   svn_ra_dav__session_t *ras = session->priv;
   int rv;
   const char *url;
- struct lock_request_baton *lrb;
   struct receiver_baton *rb;
   svn_string_t fs_path;
 
@@ -1338,16 +1339,15 @@
                                         url, SVN_INVALID_REVNUM, pool));
 
   /* Build context for neon callbacks and then register them. */
- lrb = apr_pcalloc(pool, sizeof(*lrb));
- lrb->pool = pool;
- ne_hook_create_request(ras->sess, create_request_hook, lrb);
- ne_hook_pre_send(ras->sess, pre_send_hook, lrb);
+ setup_neon_request_hook(ras);
+ memset((ras->lrb), 0, sizeof(*ras->lrb));
+ ras->lrb->pool = pool;
 
   /* Build context for the lock_receiver() callback. */
   rb = apr_pcalloc(pool, sizeof(*rb));
   rb->pool = pool;
   rb->ras = ras;
- rb->lrb = lrb;
+ rb->lrb = ras->lrb;
   rb->fs_path = fs_path.data;
 
   /* Ask neon to "discover" the lock (presumably by doing a PROPFIND
@@ -1355,39 +1355,39 @@
   rv = ne_lock_discover(ras->sess, url, lock_receiver, rb);
 
   /* Did we get a <D:error> response? */
- if (lrb->err)
+ if (ras->lrb->err)
     {
- if (lrb->error_parser)
- ne_xml_destroy(lrb->error_parser);
- return lrb->err;
+ if (ras->lrb->error_parser)
+ ne_xml_destroy(ras->lrb->error_parser);
+ return ras->lrb->err;
     }
 
   /* Did lock_receiver() generate an error? */
   if (rb->err)
     {
- if (lrb->error_parser)
- ne_xml_destroy(lrb->error_parser);
+ if (ras->lrb->error_parser)
+ ne_xml_destroy(ras->lrb->error_parser);
       return rb->err;
     }
 
   /* Did we get some other sort of neon error? */
   if (rv)
     {
- if (lrb->error_parser)
- ne_xml_destroy(lrb->error_parser);
+ if (ras->lrb->error_parser)
+ ne_xml_destroy(ras->lrb->error_parser);
       return svn_ra_dav__convert_error(ras->sess,
                                        _("Failed to fetch lock information"),
                                        rv, pool);
     }
 
   /* Free neon things. */
- if (lrb->error_parser)
- ne_xml_destroy(lrb->error_parser);
+ if (ras->lrb->error_parser)
+ ne_xml_destroy(ras->lrb->error_parser);
 
   /* Check to see if the server sent a custom 'creationdate' header in
      the PROPFIND response. If so, use it. */
- if (lrb->creation_date)
- rb->lock->creation_date = lrb->creation_date;
+ if (ras->lrb->creation_date)
+ rb->lock->creation_date = ras->lrb->creation_date;
   
   *lock = rb->lock;
   return SVN_NO_ERROR;

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org