Kannan wrote:
> Assertion failure occurred due to a non-canonical base URL passed to
> `svn_path_url_add_component2()'. Despite having canonicalized them
> wherever they are generated, the reason for this was that in
> `end_element()' of props.c, canonicalization was done where the url was
> assigned:
>
> <snip>
>
> return assign_rsrc_url(pc->rsrc, svn_uri_canonicalize(cdata, pc->pool),
> pc->pool);
Yes, because in this code path we know that the value of the variable
'cdata' is expected to be a URL.
> </snip>
>
> But there's one more place(which I missed to notice) where the value of
> `cdata'(non-canonical url) is used to assigned the URL, for those
> files/dirs who've parent info(cp/mv operations). So the non-canonical
> URL was this one and hence the failure.
OK, that's good: you've found the other place where it needs to be
canonicalized.
> Attached herewith is the patch which canonicalizes `cdata' where its
> initialized as proposed in [1].
But that's not right. Like I said before, the variable holds different
kinds of data at different times. (If we this variable was intended to
always hold a URL, we would want the variable's name to indicate so.)
It is wrong to call svn_uri_canonicalize() on a string that is not known
to be a URL. Depending on what the string is, that might change it in
some undesired way.
> Though there's one more place:
>
> <snip>
>
> case ELEM_status:
> /* Parse the <status> tag's CDATA for a status code. */
> if (ne_parse_statusline(cdata, &status))
> return svn_error_create(SVN_ERR_XML_MALFORMED, NULL, NULL);
>
> </snip>
>
> that does not need the canonicalized value, thought its better to do the
> canonicalization in just one place.
No. (And there are several other places where the variable is used in
that function, where the value it holds is something other than a URL.)
- Julian
Received on 2010-02-04 19:30:53 CET