Jon Foster wrote on Tue, Sep 21, 2010 at 00:43:13 +0100:
> Hi,
>
> Updated patch attached.
>
> [[[
> Signal SVN_ERR_BAD_OLD_VALUE over DAV using a HTTP 412 status
> code inside a 207 multi-status response.
>
> * subversion/mod_dav_svn/util.c
> (dav_svn__convert_err): Map SVN_ERR_BAD_OLD_VALUE to 412.
>
> * subversion/libsvn_ra_neon/util.c
> (multistatus_baton_t): New member contains_precondition_error.
> (end_207_element): Check for HTTP 412 status code and create
> a SVN_ERR_BAD_OLD_VALUE error if found.
>
> * subversion/libsvn_ra_serf/ra_serf.h
> (svn_ra_serf__server_error_t): New member
> contains_precondition_error.
>
> * subversion/libsvn_ra_serf/util.c
> (parse_dav_status): New method.
> (start_207): Parse DAV:status XML element.
> (end_207): Parse DAV:status XML element. Use SVN_ERR_BAD_OLD_VALUE
> error code if applicable.
> (svn_ra_serf__handle_multistatus_only): Initialise new member.
>
> Patch by: Jon Foster <jon.fos...@cabot.co.uk>
> ]]]
>
> > > Index: subversion/libsvn_ra_serf/util.c
> > > ===================================================================
> > > --- subversion/libsvn_ra_serf/util.c (revision 999102)
> > > +++ subversion/libsvn_ra_serf/util.c (working copy)
> > > @@ -836,6 +836,7 @@ svn_ra_serf__handle_discard_body(serf_request_t
> *r
> > > + server_err->contains_precondition_error = FALSE;
> >
> > (So, this is the error handler for a function that normally discards
> > its response, and the meat of the patch is in
> > svn_ra_serf__handle_multistatus_only().)
>
> After reviewing this code again, I've dropped that chunk. I've
> convinced
> myself that the variable is never used in that code path.
>
True.
Though I'm tempted to leave it in anyway; the next person to add code to
initialize server_err might copy this initalizer...
But I speculate that the thing is allocated with apr_pCalloc() anyway,
so this whole discussion doesn't matter :-)
> > > + err = svn_cstring_atoi(status_code_out, token);
> > > + if (err)
> > > + return svn_error_create(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
> > > + "Malformed DAV:status CDATA");
> > > + svn_stringbuf_setempty(buf);
> > > + return SVN_NO_ERROR;
> > > +}
> > > +
> > > /*
> > > * Expat callback invoked on a start element tag for a 207
> response.
> > > */
> > > @@ -968,6 +994,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
> > > svn_stringbuf_setempty(ctx->cdata);
> > > ctx->collect_cdata = TRUE;
> > > }
> > > + else if (ctx->in_error &&
> > > + strcmp(name.namespace, "DAV:") == 0 &&
> > > + strcmp(name.name, "status") == 0)
> > > + {
> > > + /* Start collecting cdata. */
> > > + svn_stringbuf_setempty(ctx->cdata);
> >
> > Okay; D:responsedescription and D:status are siblings [1], so it's
> > okay to reuse CTX->cdata.
> >
> > <paranoia on>
> > Are you sure they will always be siblings? If we aren't sure that
> yes,
> > we could just use two stringbufs (instead of reusing ctx->cdata).
>
> Even with the old code, if there are any elements with CDATA nested
> inside <D:responsedescription>, Serf is going to go wrong. If we
Ah, right.
> collect CDATA into different variables, then it'll just go wrong in
> a different way (e.g. the HTTP status line might be shown as part of
> the message).
>
> I think there's a whole package of work that could be done to make
> Serf resilient against weird XML, but I think that's separate from
> this work. (It's also quite hard to test).
>
Agreed.
> Thanks for the thourough review!
You're welcome.
Content-Description: patch_atomic_revprops_dav2.txt
> Index: subversion/libsvn_ra_serf/util.c
> ===================================================================
> --- subversion/libsvn_ra_serf/util.c (revision 999156)
> +++ subversion/libsvn_ra_serf/util.c (working copy)
> @@ -945,6 +945,34 @@ svn_ra_serf__handle_status_only(serf_request_t *re
> return svn_error_return(err);
> }
>
> +/* Given a string like "HTTP/1.1 500 (status)", parse out the numeric status
> + code. Ignores leading whitespace. */
Good docstring, but it will be slightly clearer (and extensible) if you
called out the parameter names:
+/* Given a string like "HTTP/1.1 500 (status)" in BUF, parse out the numeric
status
+ code into *STATUS_CODE_OUT. Ignores leading whitespace. */
(but yeah, I'm being lazy and dropping the "Use POOL for temporary
allocations" boilerplate)
> +static svn_error_t *
> +parse_dav_status(int *status_code_out, svn_stringbuf_t *buf,
> + apr_pool_t *scratch_pool)
> +{
> + svn_error_t *err;
> + const char *token;
> + char *tok_status;
> + svn_stringbuf_t *temp_buf = svn_stringbuf_create(buf->data, scratch_pool);
*cough* svn_stringbuf_dup()
> +
> + svn_stringbuf_strip_whitespace(temp_buf);
> + token = apr_strtok(temp_buf->data, " \t\r\n", &tok_status);
> + if (token)
> + token = apr_strtok(NULL, " \t\r\n", &tok_status);
> + if (!token)
> + return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, NULL,
> + "Malformed DAV:status CDATA '%s'",
> + buf->data);
> + err = svn_cstring_atoi(status_code_out, token);
> + if (err)
> + return svn_error_createf(SVN_ERR_RA_DAV_MALFORMED_DATA, err,
> + "Malformed DAV:status CDATA '%s'",
> + buf->data);
> +
> + return SVN_NO_ERROR;
> +}
> +
> /*
> * Expat callback invoked on a start element tag for a 207 response.
> */
> @@ -968,6 +996,14 @@ start_207(svn_ra_serf__xml_parser_t *parser,
> svn_stringbuf_setempty(ctx->cdata);
> ctx->collect_cdata = TRUE;
> }
> + else if (ctx->in_error &&
> + strcmp(name.namespace, "DAV:") == 0 &&
> + strcmp(name.name, "status") == 0)
> + {
> + /* Start collecting cdata. */
> + svn_stringbuf_setempty(ctx->cdata);
> + ctx->collect_cdata = TRUE;
> + }
>
> return SVN_NO_ERROR;
> }
> @@ -993,9 +1029,24 @@ end_207(svn_ra_serf__xml_parser_t *parser,
> ctx->collect_cdata = FALSE;
> ctx->error->message = apr_pstrmemdup(ctx->error->pool,
> ctx->cdata->data,
> ctx->cdata->len);
> - ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
> + if (ctx->contains_precondition_error)
> + ctx->error->apr_err = SVN_ERR_BAD_OLD_VALUE;
> + else
> + ctx->error->apr_err = SVN_ERR_RA_DAV_REQUEST_FAILED;
> }
> + else if (ctx->in_error &&
> + strcmp(name.namespace, "DAV:") == 0 &&
> + strcmp(name.name, "status") == 0)
> + {
> + int status_code;
>
> + ctx->collect_cdata = FALSE;
> +
> + SVN_ERR(parse_dav_status(&status_code, ctx->cdata, parser->pool));
> + if (status_code == 412)
> + ctx->contains_precondition_error = TRUE;
> + }
> +
> return SVN_NO_ERROR;
> }
>
> @@ -1044,6 +1095,7 @@ svn_ra_serf__handle_multistatus_only(serf_request_
> {
> server_err->error = svn_error_create(APR_SUCCESS, NULL, NULL);
> server_err->has_xml_response = TRUE;
> + server_err->contains_precondition_error = FALSE;
> server_err->cdata = svn_stringbuf_create("", pool);
> server_err->collect_cdata = FALSE;
> server_err->parser.pool = server_err->error->pool;
> Index: subversion/libsvn_ra_serf/ra_serf.h
> ===================================================================
> --- subversion/libsvn_ra_serf/ra_serf.h (revision 999156)
> +++ subversion/libsvn_ra_serf/ra_serf.h (working copy)
> @@ -673,6 +673,9 @@ typedef struct svn_ra_serf__server_error_t {
> /* Have we seen an error tag? */
> svn_boolean_t in_error;
>
> + /* Have we seen a HTTP "412 Precondition Failed" error? */
> + svn_boolean_t contains_precondition_error;
> +
> /* Should we be collecting the XML cdata? */
> svn_boolean_t collect_cdata;
>
