You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by sv...@apache.org on 2015/04/18 06:00:17 UTC
svn commit: r1674433 - in /subversion/branches/1.9.x: ./ STATUS
subversion/libsvn_ra_serf/stat.c subversion/libsvn_ra_serf/util.c
Author: svn-role
Date: Sat Apr 18 04:00:16 2015
New Revision: 1674433
URL: http://svn.apache.org/r1674433
Log:
Merge the r1665213 group from trunk:
* r1665213, r1665259, r1665609
Fix cases of serf's svn_ra_get_dir2() and svn_ra_get_log2() breaking the
ra session for further operations.
Justification:
Without this fix the next session usage after this operation may
accidentally process the result of a still outstanding request.
.
svn_ra_get_dir2() and svn_ra_get_log2() are the easy to trigger examples,
but propfind requests are just as vulnerable for keeping requests
registered after returning with some error... If there wasn't a lucky pool
cleanup inbetween further ra operations might access already freed memory,
because the request continues processing.
Notes:
If caller just passed a scratch pool for a single request things would
usually have worked fine, but if not...
Votes:
+1: rhuijben, stefan2, brane
Modified:
subversion/branches/1.9.x/ (props changed)
subversion/branches/1.9.x/STATUS
subversion/branches/1.9.x/subversion/libsvn_ra_serf/stat.c
subversion/branches/1.9.x/subversion/libsvn_ra_serf/util.c
Propchange: subversion/branches/1.9.x/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Sat Apr 18 04:00:16 2015
@@ -90,4 +90,4 @@
/subversion/branches/verify-at-commit:1462039-1462408
/subversion/branches/verify-keep-going:1439280-1546110
/subversion/branches/wc-collate-path:1402685-1480384
-/subversion/trunk:1660545-1660547,1660549-1662901,1663003,1663183-1663184,1663338,1663347,1663355,1663374,1663450,1663697,1663706,1663738,1663749,1663791,1663991,1664078,1664080,1664084-1664085,1664187,1664191,1664193,1664200,1664344,1664476,1664480-1664481,1664483,1664489-1664490,1664507,1664520-1664521,1664523,1664526-1664527,1664531-1664532,1664588,1664593-1664594,1664596,1664653,1664684,1664927,1664938-1664940,1664978,1664984,1664997,1665164,1665195,1665318,1665437-1665438,1665611-1665612,1665845,1665850,1665852,1665886,1665894,1665896,1666096,1666258,1666270,1666272,1666379,1666449,1666690,1666851,1667101,1667106-1667107,1667233,1667235,1667301,1667471,1667715,1667941,1667976,1668320,1668598-1668600,1668602-1668603,1668607-1668608,1668618,1669743,1669746,1669749,1669945,1670139,1670149,1670152,1670329,1670337,1670347,1670353,1671164,1671388,1672295,1672311,1672372,1672404,1672511-1672512,1672578,1672728,1673044,1673062-1673063,1673065,1673153,1673170,1673172,1673197,1673202,167
3204,1673228,1673282,1673445,1673691-1673692,1673746,1673785,1673803,1674015,1674032,1674170
+/subversion/trunk:1660545-1660547,1660549-1662901,1663003,1663183-1663184,1663338,1663347,1663355,1663374,1663450,1663697,1663706,1663738,1663749,1663791,1663991,1664078,1664080,1664084-1664085,1664187,1664191,1664193,1664200,1664344,1664476,1664480-1664481,1664483,1664489-1664490,1664507,1664520-1664521,1664523,1664526-1664527,1664531-1664532,1664588,1664593-1664594,1664596,1664653,1664684,1664927,1664938-1664940,1664978,1664984,1664997,1665164,1665195,1665213,1665259,1665318,1665437-1665438,1665609,1665611-1665612,1665845,1665850,1665852,1665886,1665894,1665896,1666096,1666258,1666270,1666272,1666379,1666449,1666690,1666851,1667101,1667106-1667107,1667233,1667235,1667301,1667471,1667715,1667941,1667976,1668320,1668598-1668600,1668602-1668603,1668607-1668608,1668618,1669743,1669746,1669749,1669945,1670139,1670149,1670152,1670329,1670337,1670347,1670353,1671164,1671388,1672295,1672311,1672372,1672404,1672511-1672512,1672578,1672728,1673044,1673062-1673063,1673065,1673153,1673170,167
3172,1673197,1673202,1673204,1673228,1673282,1673445,1673691-1673692,1673746,1673785,1673803,1674015,1674032,1674170
Modified: subversion/branches/1.9.x/STATUS
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/STATUS?rev=1674433&r1=1674432&r2=1674433&view=diff
==============================================================================
--- subversion/branches/1.9.x/STATUS (original)
+++ subversion/branches/1.9.x/STATUS Sat Apr 18 04:00:16 2015
@@ -38,24 +38,6 @@ Veto-blocked changes:
Approved changes:
=================
- * r1665213, r1665259, r1665609
- Fix cases of serf's svn_ra_get_dir2() and svn_ra_get_log2() breaking the
- ra session for further operations.
- Justification:
- Without this fix the next session usage after this operation may
- accidentally process the result of a still outstanding request.
- .
- svn_ra_get_dir2() and svn_ra_get_log2() are the easy to trigger examples,
- but propfind requests are just as vulnerable for keeping requests
- registered after returning with some error... If there wasn't a lucky pool
- cleanup inbetween further ra operations might access already freed memory,
- because the request continues processing.
- Notes:
- If caller just passed a scratch pool for a single request things would
- usually have worked fine, but if not...
- Votes:
- +1: rhuijben, stefan2, brane
-
* r1664664, r1664672, r1664674
Make detection of invalid (newer than HEAD) base revision on commit behave
the same on all RA layers.
Modified: subversion/branches/1.9.x/subversion/libsvn_ra_serf/stat.c
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/subversion/libsvn_ra_serf/stat.c?rev=1674433&r1=1674432&r2=1674433&view=diff
==============================================================================
--- subversion/branches/1.9.x/subversion/libsvn_ra_serf/stat.c (original)
+++ subversion/branches/1.9.x/subversion/libsvn_ra_serf/stat.c Sat Apr 18 04:00:16 2015
@@ -472,6 +472,7 @@ svn_ra_serf__get_dir(svn_ra_session_t *r
svn_ra_serf__handler_t *props_handler = NULL;
const char *path;
struct get_dir_baton_t gdb;
+ svn_error_t *err = SVN_NO_ERROR;
gdb.result_pool = result_pool;
gdb.is_directory = FALSE;
@@ -542,10 +543,17 @@ svn_ra_serf__get_dir(svn_ra_session_t *r
if (dirent_handler)
{
- SVN_ERR(svn_ra_serf__context_run_wait(&dirent_handler->done,
+ err = svn_error_trace(
+ svn_ra_serf__context_run_wait(&dirent_handler->done,
session,
scratch_pool));
+ if (err)
+ {
+ svn_pool_clear(scratch_pool); /* Unregisters outstanding requests */
+ return err;
+ }
+
if (gdb.supports_deadprop_count == svn_tristate_false
&& session->supports_deadprop_count == svn_tristate_unknown
&& dirent_fields & SVN_DIRENT_HAS_PROPS)
@@ -571,23 +579,27 @@ svn_ra_serf__get_dir(svn_ra_session_t *r
if (props_handler)
{
- SVN_ERR(svn_ra_serf__context_run_wait(&props_handler->done,
+ err = svn_error_trace(
+ svn_ra_serf__context_run_wait(&props_handler->done,
session,
scratch_pool));
}
/* And dirent again for the case when we had to send the request again */
- if (dirent_handler)
+ if (! err && dirent_handler)
{
- SVN_ERR(svn_ra_serf__context_run_wait(&dirent_handler->done,
+ err = svn_error_trace(
+ svn_ra_serf__context_run_wait(&dirent_handler->done,
session,
scratch_pool));
}
- if (gdb.supports_deadprop_count != svn_tristate_unknown)
+ if (!err && gdb.supports_deadprop_count != svn_tristate_unknown)
session->supports_deadprop_count = gdb.supports_deadprop_count;
- svn_pool_destroy(scratch_pool);
+ svn_pool_destroy(scratch_pool); /* Unregisters outstanding requests */
+
+ SVN_ERR(err);
if (!gdb.is_directory)
return svn_error_create(SVN_ERR_FS_NOT_DIRECTORY, NULL,
Modified: subversion/branches/1.9.x/subversion/libsvn_ra_serf/util.c
URL: http://svn.apache.org/viewvc/subversion/branches/1.9.x/subversion/libsvn_ra_serf/util.c?rev=1674433&r1=1674432&r2=1674433&view=diff
==============================================================================
--- subversion/branches/1.9.x/subversion/libsvn_ra_serf/util.c (original)
+++ subversion/branches/1.9.x/subversion/libsvn_ra_serf/util.c Sat Apr 18 04:00:16 2015
@@ -947,6 +947,22 @@ svn_ra_serf__context_run_wait(svn_boolea
return SVN_NO_ERROR;
}
+/* Ensure that a handler is no longer scheduled on the connection.
+
+ Eventually serf will have a reliable way to cancel existing requests,
+ but currently it doesn't even have a way to relyable identify a request
+ after rescheduling, for auth reasons.
+
+ So the only thing we can do today is reset the connection, which
+ will cancel all outstanding requests and prepare the connection
+ for re-use.
+*/
+static void
+svn_ra_serf__unschedule_handler(svn_ra_serf__handler_t *handler)
+{
+ serf_connection_reset(handler->conn->conn);
+ handler->scheduled = FALSE;
+}
svn_error_t *
svn_ra_serf__context_run_one(svn_ra_serf__handler_t *handler,
@@ -960,6 +976,15 @@ svn_ra_serf__context_run_one(svn_ra_serf
/* Wait until the response logic marks its DONE status. */
err = svn_ra_serf__context_run_wait(&handler->done, handler->session,
scratch_pool);
+
+ if (handler->scheduled)
+ {
+ /* We reset the connection (breaking pipelining, etc.), as
+ if we didn't the next data would still be handled by this handler,
+ which is done as far as our caller is concerned. */
+ svn_ra_serf__unschedule_handler(handler);
+ }
+
return svn_error_trace(err);
}
@@ -1455,7 +1480,13 @@ handle_response_cb(serf_request_t *reque
{
handler->discard_body = TRUE; /* Discard further data */
handler->done = TRUE; /* Mark as done */
- handler->scheduled = FALSE;
+ /* handler->scheduled is still TRUE, as we still expect data.
+ If we would return an error outer-status the connection
+ would have to be restarted. With scheduled still TRUE
+ destroying the handler's pool will still reset the
+ connection, avoiding the posibility of returning
+ an error for this handler when a new request is
+ scheduled. */
outer_status = APR_EAGAIN; /* Exit context loop */
}
@@ -1883,14 +1914,14 @@ response_done(serf_request_t *request,
handlers registered in no freed memory.
This fallback kills the connection for this case, which will make serf
- unregister any*/
+ unregister any outstanding requests on it. */
static apr_status_t
handler_cleanup(void *baton)
{
svn_ra_serf__handler_t *handler = baton;
- if (handler->scheduled && handler->conn && handler->conn->conn)
+ if (handler->scheduled)
{
- serf_connection_reset(handler->conn->conn);
+ svn_ra_serf__unschedule_handler(handler);
}
return APR_SUCCESS;