cmpilato@tigris.org writes:
> Log:
> Finish issue #2335.
>
> * subversion/libsvn_ra_dav/merge.c
> (svn_ra_dav__assemble_locktoken_body): XML-escape lock paths used in
> MERGE requests.
>
> * subversion/tests/clients/cmdline/lock_tests.py
> (commit_xml_unsafe_file_unlock): New regression test for this defect.
> (test_funcs): Add reference to new test.
>
> --- trunk/subversion/libsvn_ra_dav/merge.c (original)
> +++ trunk/subversion/libsvn_ra_dav/merge.c Thu Jun 30 16:02:32 2005
> @@ -550,39 +550,51 @@
> apr_size_t buf_size;
> const char *closing_tag = "</S:lock-token-list>";
> apr_size_t closing_tag_size = strlen(closing_tag);
> - svn_stringbuf_t *lockbuf =
> - svn_stringbuf_create
> - ("<S:lock-token-list xmlns:S=\"" SVN_XML_NAMESPACE "\">", pool);
> + apr_pool_t *tmppool = svn_pool_create(pool);
> + apr_hash_t *xml_locks = apr_hash_make(tmppool);
> + svn_stringbuf_t *lockbuf = svn_stringbuf_create
> + ("<S:lock-token-list xmlns:S=\"" SVN_XML_NAMESPACE "\">" DEBUG_CR, pool);
>
> buf_size = lockbuf->len;
Hmm, 'tmppool', that's unusual. It doesn't appear to be a standard
sort of loop pool -- is it an exception to our usual pool policy?
(Ah, question answered below, see there.)
> -#define SVN_LOCK "<S:lock>"
> +#define SVN_LOCK "<S:lock>" DEBUG_CR
> #define SVN_LOCK_LEN sizeof(SVN_LOCK)-1
> -#define SVN_LOCK_CLOSE "</S:lock>"
> +#define SVN_LOCK_CLOSE "</S:lock>" DEBUG_CR
> #define SVN_LOCK_CLOSE_LEN sizeof(SVN_LOCK_CLOSE)-1
> #define SVN_LOCK_PATH "<S:lock-path>"
> #define SVN_LOCK_PATH_LEN sizeof(SVN_LOCK_PATH)-1
> -#define SVN_LOCK_PATH_CLOSE "</S:lock-path>"
> +#define SVN_LOCK_PATH_CLOSE "</S:lock-path>" DEBUG_CR
> #define SVN_LOCK_PATH_CLOSE_LEN sizeof(SVN_LOCK_CLOSE)-1
> #define SVN_LOCK_TOKEN "<S:lock-token>"
> #define SVN_LOCK_TOKEN_LEN sizeof(SVN_LOCK_TOKEN)-1
> -#define SVN_LOCK_TOKEN_CLOSE "</S:lock-token>"
> +#define SVN_LOCK_TOKEN_CLOSE "</S:lock-token>" DEBUG_CR
> #define SVN_LOCK_TOKEN_CLOSE_LEN sizeof(SVN_LOCK_TOKEN_CLOSE)-1
>
> /* First, figure out how much string data we're talking about,
> and allocate a stringbuf big enough to hold it all... we *never*
> - want have the stringbuf do an auto-reallocation. */
> + want have the stringbuf do an auto-reallocation. While here,
> + we'll be copying our hash of paths -> tokens into a hash of
> + xml-escaped-paths -> tokens. */
Seems like an overly vehement comment, but that's just a matter of
taste I guess :-). It wouldn't be horrible to do auto-allocation,
though it's even nicer to anticipate the right size in advance.
> for (hi = apr_hash_first(pool, lock_tokens); hi; hi = apr_hash_next(hi))
> {
> const void *key;
> void *val;
> apr_ssize_t klen;
> -
> + svn_string_t lock_path;
> + svn_stringbuf_t *lock_path_xml = NULL;
> +
> apr_hash_this(hi, &key, &klen, &val);
> +
> + /* XML-escape our key and store it in our temporary hash. */
> + lock_path.data = key;
> + lock_path.len = klen;
> + svn_xml_escape_cdata_string(&lock_path_xml, &lock_path, tmppool);
> + apr_hash_set(xml_locks, lock_path_xml->data, lock_path_xml->len, val);
>
> + /* Now, on with the stringbuf calculations. */
> buf_size += SVN_LOCK_LEN;
> buf_size += SVN_LOCK_PATH_LEN;
> - buf_size += klen;
> + buf_size += lock_path_xml->len;
> buf_size += SVN_LOCK_PATH_CLOSE_LEN;
> buf_size += SVN_LOCK_TOKEN_LEN;
> buf_size += strlen(val);
> @@ -594,21 +606,22 @@
>
> svn_stringbuf_ensure(lockbuf, buf_size + 1);
>
> - /* Now append all the hash's keys and values into the stringbuf.
> - This is better than doing apr_pstrcat() in a loop, because
> - (1) there's no need to constantly re-alloc, and (2) the
> + /* Now append all the temporary hash's keys and values into the
> + stringbuf. This is better than doing apr_pstrcat() in a loop,
> + because (1) there's no need to constantly re-alloc, and (2) the
> stringbuf already knows the end of the buffer, so there's no
> seek-time to the end of the string when appending. */
> - for (hi = apr_hash_first(pool, lock_tokens); hi; hi = apr_hash_next(hi))
> + for (hi = apr_hash_first(pool, xml_locks); hi; hi = apr_hash_next(hi))
> {
> const void *key;
> + apr_ssize_t klen;
> void *val;
>
> - apr_hash_this(hi, &key, NULL, &val);
> + apr_hash_this(hi, &key, &klen, &val);
>
> svn_stringbuf_appendcstr(lockbuf, SVN_LOCK);
> svn_stringbuf_appendcstr(lockbuf, SVN_LOCK_PATH);
> - svn_stringbuf_appendcstr(lockbuf, key);
> + svn_stringbuf_appendbytes(lockbuf, key, klen);
> svn_stringbuf_appendcstr(lockbuf, SVN_LOCK_PATH_CLOSE);
> svn_stringbuf_appendcstr(lockbuf, SVN_LOCK_TOKEN);
> svn_stringbuf_appendcstr(lockbuf, val);
Okay, I see what's going on. We can't use a standard loop pool,
because we need all the data for a *second* loop, after the first loop
ends. And the second loop doesn't need a pool, since it just puts
together the stringbuf.
But, as long as you're using tmppool like this, why not use it in the
calls to apr_hash_first() as well? If we're going to clear out the
memory we used, we might as clear as much of it as possible.
> @@ -632,6 +645,8 @@
> #undef SVN_LOCK_TOKEN_CLOSE_LEN
>
> *body = lockbuf;
> +
> + svn_pool_destroy(tmppool);
> return SVN_NO_ERROR;
> }
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Fri Jul 1 20:59:17 2005