Semi-related question: how does this fix interact with this part of
svnserve's main():
/* Use a subpool for the connection to ensure that if SASL is used
* the pool cleanup handlers that call sasl_dispose() (connection_pool)
* and sasl_done() (pool) are run in the right order. See issue #3664. */
connection_pool = svn_pool_create(pool);
conn = svn_ra_svn_create_conn2(NULL, in_file, out_file,
params.compression_level,
connection_pool);
svn_error_clear(serve(conn, ¶ms, connection_pool));
exit(0);
?
Both are SASL pool lifetime issues. Is the above hunk still needed
after the change below?
Philip Martin wrote on Thu, Sep 08, 2011 at 11:20:00 +0100:
> "Bert Huijben" <b...@qqmail.nl> writes:
>
> >> --- subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c (original)
> >> +++ subversion/trunk/subversion/libsvn_ra_svn/cyrus_auth.c Thu Sep 8
> >> 08:05:00 2011
> >> @@ -738,9 +738,7 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__se
> >> const char *mechstring = "", *last_err = "", *realmstring;
> >> const char *local_addrport = NULL, *remote_addrport = NULL;
> >> svn_boolean_t success;
> >> - /* Reserve space for 3 callbacks (for the username, password and the
> >> - array terminator). */
> >> - sasl_callback_t callbacks[3];
> >> + sasl_callback_t *callbacks;
> >> cred_baton_t cred_baton;
> >> int i;
> >>
> >> @@ -776,6 +774,10 @@ svn_ra_svn__do_cyrus_auth(svn_ra_svn__se
> >> cred_baton.realmstring = realmstring;
> >> cred_baton.pool = pool;
> >>
> >> + /* Reserve space for 3 callbacks (for the username, password and the
> >> + array terminator). */
> >> + callbacks = apr_palloc(sess->conn->pool, sizeof(*callbacks) * 3);
> >> +
> >> /* Initialize the callbacks array. */
> >
> > This isn't going to help when the baton that is passed (by pointer) to
> > the callbacks is also allocated on the stack. (The baton should
> > probably move to heap as well if this is the right fix)
>
> In practice it doesn't matter, see below. We should probably change it
> or add a comment.
>
> > The function seems to assume that this callback infrastructure isn't
> > used after returning from svn_ra_svn__do_cyrus_auth(), which would
> > make allocating on the stack safe.
> >
> > Any idea why this worked for years in 1.6 but now starts failing?
>
> The original bug report explained it. During pool cleanup we call the
> SASL function sasl_dispose and that looks at the callback structs. The
> stack struct has random values and if the id member makes it look like
> the logging callback it gets invoked. Most callbacks won't get invoked
> at that stage so only particular values will cause a problem. Also
> since the auth callback won't get invoked it explains why the baton on
> the stack works.
>
> --
> uberSVN: Apache Subversion Made Easy
> http://www.uberSVN.com
