You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by rh...@apache.org on 2013/07/03 15:13:49 UTC
svn commit: r1499386 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h util.c xml.c
Author: rhuijben
Date: Wed Jul 3 13:13:49 2013
New Revision: 1499386
URL: http://svn.apache.org/r1499386
Log:
Improve transition based xml parser validations a bit by checking for opening
and closing of a document element. Also improve an error message for this
parser, to help me in debugging.
* subversion/libsvn_ra_serf/ra_serf.h
(svn_ra_serf__xml_context_destroy): Rename to ...
(svn_ra_serf__xml_context_done): ... this.
* subversion/libsvn_ra_serf/util.c
(expat_response_handler): Avoid duplicated code by using an at_eof flag.
Call svn_ra_serf__xml_context_done() to verify clean parser shutdown.
* subversion/libsvn_ra_serf/xml.c
(svn_ra_serf__xml_context_destroy): Rename to...
(svn_ra_serf__xml_context_done): ... this and add verification on the
final state of the transition parser.
(svn_ra_serf__xml_cb_start): Improve error message.
Modified:
subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
subversion/trunk/subversion/libsvn_ra_serf/util.c
subversion/trunk/subversion/libsvn_ra_serf/xml.c
Modified: subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h?rev=1499386&r1=1499385&r2=1499386&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/ra_serf.h Wed Jul 3 13:13:49 2013
@@ -801,10 +801,10 @@ svn_ra_serf__xml_context_create(
void *baton,
apr_pool_t *result_pool);
-/* Destroy all subpools for this structure. */
-void
-svn_ra_serf__xml_context_destroy(
- svn_ra_serf__xml_context_t *xmlctx);
+/* Verifies if the parsing completed successfully and destroys
+ all subpools. */
+svn_error_t *
+svn_ra_serf__xml_context_done(svn_ra_serf__xml_context_t *xmlctx);
/* Construct a handler with the response function/baton set up to parse
a response body using the given XML context. The handler and its
Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1499386&r1=1499385&r2=1499386&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/util.c Wed Jul 3 13:13:49 2013
@@ -2540,10 +2540,13 @@ expat_response_handler(serf_request_t *r
const char *data;
apr_size_t len;
int expat_status;
+ svn_boolean_t at_eof = FALSE;
status = serf_bucket_read(response, PARSE_CHUNK_SIZE, &data, &len);
if (SERF_BUCKET_READ_ERROR(status))
return svn_ra_serf__wrap_err(status, NULL);
+ else if (APR_STATUS_IS_EOF(status))
+ at_eof = TRUE;
#if 0
/* ### move restart/skip into the core handler */
@@ -2555,7 +2558,15 @@ expat_response_handler(serf_request_t *r
/* ### should we have an IGNORE_ERRORS flag like the v1 parser? */
- expat_status = XML_Parse(ectx->parser, data, (int)len, 0 /* isFinal */);
+ expat_status = XML_Parse(ectx->parser, data, (int)len,
+ at_eof /* isFinal */);
+
+ if (at_eof)
+ {
+ /* Release xml parser state/tables. */
+ apr_pool_cleanup_run(ectx->cleanup_pool, &ectx->parser,
+ xml_parser_cleanup);
+ }
/* We need to check INNER_ERROR first. This is an error from the
callbacks that has been "dropped off" for us to retrieve. On
@@ -2564,12 +2575,7 @@ expat_response_handler(serf_request_t *r
If an error is not present, THEN we go ahead and look for parsing
errors. */
- if (ectx->inner_error)
- {
- apr_pool_cleanup_run(ectx->cleanup_pool, &ectx->parser,
- xml_parser_cleanup);
- return svn_error_trace(ectx->inner_error);
- }
+ SVN_ERR(ectx->inner_error);
if (expat_status == XML_STATUS_ERROR)
return svn_error_createf(SVN_ERR_XML_MALFORMED,
ectx->inner_error,
@@ -2581,18 +2587,10 @@ expat_response_handler(serf_request_t *r
/* The parsing went fine. What has the bucket told us? */
- if (APR_STATUS_IS_EOF(status))
+ if (at_eof)
{
- /* Tell expat we've reached the end of the content. Ignore the
- return status. We just don't care. */
- (void) XML_Parse(ectx->parser, NULL, 0, 1 /* isFinal */);
-
- svn_ra_serf__xml_context_destroy(ectx->xmlctx);
- apr_pool_cleanup_run(ectx->cleanup_pool, &ectx->parser,
- xml_parser_cleanup);
-
- /* ### should check XMLCTX to see if it has returned to the
- ### INITIAL state. we may have ended early... */
+ /* Make sure we actually got xml and clean up after parsing */
+ SVN_ERR(svn_ra_serf__xml_context_done(ectx->xmlctx));
}
if (status && !SERF_BUCKET_READ_ERROR(status))
Modified: subversion/trunk/subversion/libsvn_ra_serf/xml.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/xml.c?rev=1499386&r1=1499385&r2=1499386&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_ra_serf/xml.c (original)
+++ subversion/trunk/subversion/libsvn_ra_serf/xml.c Wed Jul 3 13:13:49 2013
@@ -458,11 +458,50 @@ lazy_create_pool(void *baton)
return xes->state_pool;
}
-void
-svn_ra_serf__xml_context_destroy(
- svn_ra_serf__xml_context_t *xmlctx)
+svn_error_t *
+svn_ra_serf__xml_context_done(svn_ra_serf__xml_context_t *xmlctx)
{
+ if (xmlctx->current->prev)
+ {
+ /* Probably unreachable as this would be an xml parser error */
+ return svn_error_createf(SVN_ERR_XML_MALFORMED, NULL,
+ _("XML stream truncated: closing '%s' missing"),
+ xmlctx->current->tag.name);
+ }
+ else if (! xmlctx->free_states)
+ {
+ /* If we have no items on the free_states list, we didn't push anything,
+ which tells us that we found an empty xml body */
+ const svn_ra_serf__xml_transition_t *scan;
+ const svn_ra_serf__xml_transition_t *document = NULL;
+ const char *msg;
+
+ for (scan = xmlctx->ttable; scan->ns != NULL; ++scan)
+ {
+ if (scan->from_state == XML_STATE_INITIAL)
+ {
+ if (document != NULL)
+ {
+ document = NULL; /* Multiple document elements defined */
+ break;
+ }
+ document = scan;
+ }
+ }
+
+ if (document)
+ msg = apr_psprintf(xmlctx->scratch_pool, "'%s' element not found",
+ document->name);
+ else
+ msg = _("document element not found");
+
+ return svn_error_createf(SVN_ERR_XML_MALFORMED, NULL,
+ _("XML stream truncated: %s"),
+ msg);
+ }
+
svn_pool_destroy(xmlctx->scratch_pool);
+ return SVN_NO_ERROR;
}
svn_ra_serf__xml_context_t *
@@ -677,10 +716,11 @@ svn_ra_serf__xml_cb_start(svn_ra_serf__x
name = *saveattr;
value = svn_xml_get_attr_value(name, attrs);
if (value == NULL)
- return svn_error_createf(SVN_ERR_XML_ATTRIB_NOT_FOUND,
- NULL,
- _("Missing XML attribute: '%s'"),
- name);
+ return svn_error_createf(
+ SVN_ERR_XML_ATTRIB_NOT_FOUND,
+ NULL,
+ _("Missing XML attribute '%s' on '%s' element"),
+ name, scan->name);
}
if (value)
Re: svn commit: r1499386 - in /subversion/trunk/subversion/libsvn_ra_serf:
ra_serf.h util.c xml.c
Posted by Greg Stein <gs...@gmail.com>.
On Wed, Jul 3, 2013 at 9:13 AM, <rh...@apache.org> wrote:
>...
> +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Wed Jul 3 13:13:49 2013
>...
> @@ -2564,12 +2575,7 @@ expat_response_handler(serf_request_t *r
>
> If an error is not present, THEN we go ahead and look for parsing
> errors. */
> - if (ectx->inner_error)
> - {
> - apr_pool_cleanup_run(ectx->cleanup_pool, &ectx->parser,
> - xml_parser_cleanup);
> - return svn_error_trace(ectx->inner_error);
> - }
> + SVN_ERR(ectx->inner_error);
Why the removal of the cleanup?
>...
> +++ subversion/trunk/subversion/libsvn_ra_serf/xml.c Wed Jul 3 13:13:49 2013
>...
> + if (document)
> + msg = apr_psprintf(xmlctx->scratch_pool, "'%s' element not found",
> + document->name);
> + else
> + msg = _("document element not found");
> +
> + return svn_error_createf(SVN_ERR_XML_MALFORMED, NULL,
> + _("XML stream truncated: %s"),
> + msg);
You could end up with mixed language here. "$TRANS(XML stream
truncated: )$ENGLISH('FOO' element not found)"
It seems that you'd want to have two separate svn_error_createf()
calls, with different args. That'll get the translation stuff right.
>...
Cheers,
-g