You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by VK Sameer <sa...@collab.net> on 2004/12/06 11:07:21 UTC
[PATCH] First cut at 1954 solution
Hello,
This is a first cut at solving issue 1954 (). The code is based on Jon
Foster's detailed patch - [RFC] Checking paths are legal
(http://svn.haxx.se/dev/archive-2004-09/0047.shtml). I have not picked
up all of Jon's Win32 code since I don't yet have a Windows XP box to
test the changes.
Please let me know if the changes are generally in the right direction.
make check passes after tab-related test in commit_uri_unsafe() in
commit.py was moved to tab_test(). It is at a coarse level using XFail,
instead of looking for specific error messages. Any pointers to the
right test functions to use?
Thanks
Sameer
Re: [PATCH] First cut at 1954 solution
Posted by VK Sameer <sa...@collab.net>.
On Tue, 2004-12-07 at 01:46, Peter N. Lundblad wrote:
> On Mon, 6 Dec 2004, C. Michael Pilato wrote:
>
> > VK Sameer <sa...@collab.net> writes:
> >
> > > + * First check whether all characters are valid in UTF8
> > > + * Next check whether all characters are non-control characters
> > > + * Currently, all other paths are allowed.
> >
> > Hrm... this is somewhere between "leaking the details of the
> > implmentation" and "actually useful information". I think it's the
> > "first...next..." that's bugging me.
> >
> Just documenting what's a valid path would be better.
OK, will do.
> > > +svn_error_t *svn_path_is_valid_in_svn (const char *path,
> > > + apr_size_t len,
> > > + apr_pool_t *pool);
>
> To me, this name sounds like a question that returns a boolean. Would
> replacing is with ensure make sense?
How about verify instead? Ensure seems to have connotations of modifying
the path if it's not valid.
Thanks
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 6 Dec 2004, C. Michael Pilato wrote:
> VK Sameer <sa...@collab.net> writes:
>
> > Index: subversion/include/svn_path.h
> > ===================================================================
> > --- subversion/include/svn_path.h (revision 12179)
> > +++ subversion/include/svn_path.h (working copy)
> > @@ -338,6 +338,22 @@
> > svn_boolean_t svn_path_is_backpath_present (const char *path);
> >
> >
> > + * First check whether all characters are valid in UTF8
> > + * Next check whether all characters are non-control characters
> > + * Currently, all other paths are allowed.
>
> Hrm... this is somewhere between "leaking the details of the
> implmentation" and "actually useful information". I think it's the
> "first...next..." that's bugging me.
>
Just documenting what's a valid path would be better.
> > + *
> > +svn_error_t *svn_path_is_valid_in_svn (const char *path,
> > + apr_size_t len,
> > + apr_pool_t *pool);
To me, this name sounds like a question that returns a boolean. Would
replacing is with ensure make sense?
Regards,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by kf...@collab.net.
"C. Michael Pilato" <cm...@collab.net> writes:
> I'm a big fan of APIs that list every return value, but haven't the
> time or energy to retroactively do this for Subversion. That said,
> we are pretty good about listing return codes that we anticipate
> callers explicitly testing for. I anticipate callers testing the
> return value of this function (beyond just "is errorful").
We also have a policy of documenting any error codes that callers test
explicitly.
That is, if function A calls function B, and B can return a variety of
errors, and A tests for some of those errors, then B documents at
least those specific error codes and the circumstances under which
they can be returned.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by VK Sameer <sa...@collab.net>.
Sorry, had Evolution problems (still in the primordial ooze :)) and this
didn't get sent.
On Tue, 2004-12-07 at 07:50, C. Michael Pilato wrote:
> VK Sameer <sa...@collab.net> writes:
>
> > OK, I'm open to suggestions :) If it helps any, the first comment is
> > going to go away partially as per Philip Martin's and Peter Lundblad's
> > comments.
>
> Yeah, I think someone else suggested that simply defining what a valid
> path is would do the trick (instead of detailing the algorithm for
> detecting invalid ones).
[I'm still trying to decide whether to answer each email and repeat or
consolidate and risk losing part of the thread info somewhere.]
OK, have changed as per Karl's suggestion.
> I'm a big fan of APIs that list every return value, but haven't the
> time or energy to retroactively do this for Subversion. That said,
> we are pretty good about listing return codes that we anticipate
> callers explicitly testing for. I anticipate callers testing the
> return value of this function (beyond just "is errorful").
There are 2 functions now, one that just returns a boolean value and
another that returns an error, again per Karl's suggestion.
> In fact, since the goal of the function is to answer a question -- "Is
> this path a valid Subversion path?" -- we may instead want this
> interface to take an svn_boolean_t * parameter which is populated with
> TRUE or FALSE to yield the answer, and leave all error conditions to
> mean, "Something bad happened that prevents me from answering your
> question." Besides a better semantic feel, this would also prevent
> Subversion from instantiating an error that the caller might choose
> not to use (and have to tear down).
Makes sense. I think having 2 functions as mentioned above covers your
points?
> > > Also, we don't generally pass lengths with our paths.
> >
> > I can remove that if that is the general policy.
>
> Yep, that's the general policy.
OK, done.
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by "C. Michael Pilato" <cm...@collab.net>.
VK Sameer <sa...@collab.net> writes:
> OK, I'm open to suggestions :) If it helps any, the first comment is
> going to go away partially as per Philip Martin's and Peter Lundblad's
> comments.
Yeah, I think someone else suggested that simply defining what a valid
path is would do the trick (instead of detailing the algorithm for
detecting invalid ones).
> > > +svn_error_t *svn_path_is_valid_in_svn (const char *path,
> > > + apr_size_t len,
> > > + apr_pool_t *pool);
> > > +
> >
> > Judging by the signature, I'm guessing that the function returns
> > SVN_NO_ERROR if PATH is all good, and some other error otherwise. You
> > might want to state as much, so callers of the API can tell the
> > difference between an "unsupported path" error and some other error.
>
> OK, makes sense. I wasn't sure of the convention since not all functions
> seem to have the set of possible return values documented.
I'm a big fan of APIs that list every return value, but haven't the
time or energy to retroactively do this for Subversion. That said,
we are pretty good about listing return codes that we anticipate
callers explicitly testing for. I anticipate callers testing the
return value of this function (beyond just "is errorful").
In fact, since the goal of the function is to answer a question -- "Is
this path a valid Subversion path?" -- we may instead want this
interface to take an svn_boolean_t * parameter which is populated with
TRUE or FALSE to yield the answer, and leave all error conditions to
mean, "Something bad happened that prevents me from answering your
question." Besides a better semantic feel, this would also prevent
Subversion from instantiating an error that the caller might choose
not to use (and have to tear down).
> > Also, we don't generally pass lengths with our paths.
>
> I can remove that if that is the general policy.
Yep, that's the general policy.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by VK Sameer <sa...@collab.net>.
On Mon, 2004-12-06 at 19:48, C. Michael Pilato wrote:
> VK Sameer <sa...@collab.net> writes:
>
> > Index: subversion/include/svn_path.h
> > ===================================================================
> > +/**
> > + * @since New in 1.2.
> > + *
> > + * Test to see if a path is allowed in a Subversion repository.
^^^^^^^^^^
>
> Actually, the question is whether or not a path is allowed in a
> Subversion working copy. But the generic statement is more like,
> "Check @a path to see if it is supported as the path of a versioned
> resource by Subvesion."
Right, the path has to exist in the wc before testing this fix.
I'll change the comment since I also forgot about the doxygen
requirements. The comment is regarding the repository, not the wc, btw.
> > + * First check whether all characters are valid in UTF8
> > + * Next check whether all characters are non-control characters
> > + * Currently, all other paths are allowed.
>
> Hrm... this is somewhere between "leaking the details of the
> implmentation" and "actually useful information". I think it's the
> "first...next..." that's bugging me.
OK, I'm open to suggestions :) If it helps any, the first comment is
going to go away partially as per Philip Martin's and Peter Lundblad's
comments.
> > + * This is OS-independent - it is used to decide what paths may be
> > + * added to a repository. Adding occurs either using a file on disk
> > + * (svn import/add) or one not on disk (svn mv/copy URL1 URL2).
> > + */
>
> Lose this. Subversion APIs are OS-independent unless otherwise
> stated, and it's entirely unimportant when adding occurs.
OK, will take it out.
Actually, the reason for the comment is that Jon Foster's patch has
another os-specific function, svn_path_is_valid_on_disk(), to catch
pathnames reserved by Windows. I didn't add that yet because of the lack
of a Windows test box.
> > +svn_error_t *svn_path_is_valid_in_svn (const char *path,
> > + apr_size_t len,
> > + apr_pool_t *pool);
> > +
>
> Judging by the signature, I'm guessing that the function returns
> SVN_NO_ERROR if PATH is all good, and some other error otherwise. You
> might want to state as much, so callers of the API can tell the
> difference between an "unsupported path" error and some other error.
OK, makes sense. I wasn't sure of the convention since not all functions
seem to have the set of possible return values documented.
> Also, we don't generally pass lengths with our paths.
I can remove that if that is the general policy. I'm no security expert,
but from what I've read, it's considered more secure to use
"fixed-length" functions like strncmp instead of while (*c).
If that is ok, then it seems better to do a strlen as close to the user
input as possible and pass the length around, rather than re-calculate
it at a lower level. In any case, I'm open to correction.
> > Index: subversion/libsvn_subr/path.c
> > ===================================================================
> > --- subversion/libsvn_subr/path.c (revision 12179)
> > +++ subversion/libsvn_subr/path.c (working copy)
> > @@ -29,6 +29,7 @@
> > #include "svn_private_config.h" /* for SVN_PATH_LOCAL_SEPARATOR */
> > #include "svn_utf.h"
> > #include "svn_io.h" /* for svn_io_stat() */
> > +#include "svn_ctype.h" /* for svn_ctype_() */
> >
> >
> > /* The canonical empty path. Can this be changed? Well, change the empty
> > @@ -1248,3 +1249,25 @@
> > else
> > return svn_utf_cstring_to_utf8 (path_utf8, path_apr, pool);
> > }
> > +
> > +svn_error_t*
> > +svn_path_is_valid_in_svn (const char *path,
> > + apr_size_t len,
> > + apr_pool_t *pool)
> > +{
> > + int i;
> > + for (i = 0; i < len; i++)
> > + {
>
> Yep, no need for that LEN parameter.
See above for reasoning.
Thanks for the close review, Mike.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by "C. Michael Pilato" <cm...@collab.net>.
VK Sameer <sa...@collab.net> writes:
> Index: subversion/include/svn_path.h
> ===================================================================
> --- subversion/include/svn_path.h (revision 12179)
> +++ subversion/include/svn_path.h (working copy)
> @@ -338,6 +338,22 @@
> svn_boolean_t svn_path_is_backpath_present (const char *path);
>
>
> +/**
> + * @since New in 1.2.
> + *
> + * Test to see if a path is allowed in a Subversion repository.
Actually, the question is whether or not a path is allowed in a
Subversion working copy. But the generic statement is more like,
"Check @a path to see if it is supported as the path of a versioned
resource by Subvesion."
> + * First check whether all characters are valid in UTF8
> + * Next check whether all characters are non-control characters
> + * Currently, all other paths are allowed.
Hrm... this is somewhere between "leaking the details of the
implmentation" and "actually useful information". I think it's the
"first...next..." that's bugging me.
> + *
> + * This is OS-independent - it is used to decide what paths may be
> + * added to a repository. Adding occurs either using a file on disk
> + * (svn import/add) or one not on disk (svn mv/copy URL1 URL2).
> + */
Lose this. Subversion APIs are OS-independent unless otherwise
stated, and it's entirely unimportant when adding occurs.
> +svn_error_t *svn_path_is_valid_in_svn (const char *path,
> + apr_size_t len,
> + apr_pool_t *pool);
> +
Judging by the signature, I'm guessing that the function returns
SVN_NO_ERROR if PATH is all good, and some other error otherwise. You
might want to state as much, so callers of the API can tell the
difference between an "unsupported path" error and some other error.
Also, we don't generally pass lengths with our paths.
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 12179)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -29,6 +29,7 @@
> #include "svn_private_config.h" /* for SVN_PATH_LOCAL_SEPARATOR */
> #include "svn_utf.h"
> #include "svn_io.h" /* for svn_io_stat() */
> +#include "svn_ctype.h" /* for svn_ctype_() */
>
>
> /* The canonical empty path. Can this be changed? Well, change the empty
> @@ -1248,3 +1249,25 @@
> else
> return svn_utf_cstring_to_utf8 (path_utf8, path_apr, pool);
> }
> +
> +svn_error_t*
> +svn_path_is_valid_in_svn (const char *path,
> + apr_size_t len,
> + apr_pool_t *pool)
> +{
> + int i;
> + for (i = 0; i < len; i++)
> + {
Yep, no need for that LEN parameter.
const char *c = path;
while (*c)
{
if (!svn_ctype_isutf8(*c))
...
c++;
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by VK Sameer <sa...@collab.net>.
On Tue, 2004-12-07 at 01:41, Peter N. Lundblad wrote:
> On Mon, 6 Dec 2004, Philip Martin wrote:
>
> > VK Sameer <sa...@collab.net> writes:
> >
> > > +svn_error_t*
> > > +svn_path_is_valid_in_svn (const char *path,
> > > + apr_size_t len,
> > > + apr_pool_t *pool)
> > > +{
> > > + int i;
> > > + for (i = 0; i < len; i++)
> > > + {
> > > + if (!svn_ctype_isutf8(path[i]))
> > > + return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> > > + "Invalid UTF8 character in '%s'",
> > > + svn_path_local_style (path, pool));
> >
> > UTF-8 is a multibyte encoding so this doesn't ensure that the path is
> > valid UTF-8. We already have code to check for valid UTF-8, see
> > utf_validate.c, and it's already in use. The above is either
> > redundant if combined with the existing code, or insufficient if it
> > replaces the existing code.
> >
> > I think you should either drop attempts to test UTF-8 or build on the
> > existing UTF-8 validation code.
> >
> It is redundant, since we already do this check when converting from
> native to UTF8, even if native is UTF8. Validating input on the server
> side when expecting valid UTF8 wouldn't hurt, though.
OK, will look into splitting function into client-side and server-side
checks, without and with utf8 checks, respectively.
Thanks
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 6 Dec 2004, Philip Martin wrote:
> VK Sameer <sa...@collab.net> writes:
>
> > +svn_error_t*
> > +svn_path_is_valid_in_svn (const char *path,
> > + apr_size_t len,
> > + apr_pool_t *pool)
> > +{
> > + int i;
> > + for (i = 0; i < len; i++)
> > + {
> > + if (!svn_ctype_isutf8(path[i]))
> > + return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> > + "Invalid UTF8 character in '%s'",
> > + svn_path_local_style (path, pool));
>
> UTF-8 is a multibyte encoding so this doesn't ensure that the path is
> valid UTF-8. We already have code to check for valid UTF-8, see
> utf_validate.c, and it's already in use. The above is either
> redundant if combined with the existing code, or insufficient if it
> replaces the existing code.
>
> I think you should either drop attempts to test UTF-8 or build on the
> existing UTF-8 validation code.
>
It is redundant, since we already do this check when converting from
native to UTF8, even if native is UTF8. Validating input on the server
side when expecting valid UTF8 wouldn't hurt, though.
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by Philip Martin <ph...@codematters.co.uk>.
VK Sameer <sa...@collab.net> writes:
> +svn_error_t*
> +svn_path_is_valid_in_svn (const char *path,
> + apr_size_t len,
> + apr_pool_t *pool)
> +{
> + int i;
> + for (i = 0; i < len; i++)
> + {
> + if (!svn_ctype_isutf8(path[i]))
> + return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> + "Invalid UTF8 character in '%s'",
> + svn_path_local_style (path, pool));
UTF-8 is a multibyte encoding so this doesn't ensure that the path is
valid UTF-8. We already have code to check for valid UTF-8, see
utf_validate.c, and it's already in use. The above is either
redundant if combined with the existing code, or insufficient if it
replaces the existing code.
I think you should either drop attempts to test UTF-8 or build on the
existing UTF-8 validation code.
> +
> + if (svn_ctype_iscntrl(path[i]))
> + return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> + "Invalid control character in '%s'",
> + svn_path_local_style (path, pool));
> + }
> +
> + return SVN_NO_ERROR;
> +}
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by kf...@collab.net.
VK Sameer <sa...@collab.net> writes:
> Thanks for the careful review. Considering the number of scattered
> changes, I'm glad the code is being inspected exhaustively.
You're welcome. And I'm not exhausted yet -- just *try* to exhaust
me! :-)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by VK Sameer <sa...@collab.net>.
On Tue, 2004-12-07 at 07:40, kfogel@collab.net wrote:
> VK Sameer <sa...@collab.net> writes:
>
> Run 'svn log' on the Subversion trunk to see examples of our usual log
> formatting style. See also the section "Writing log messages" in the
> HACKING file.
Sorry, I didn't read it carefully enough.
> Here is how I would write it:
>
> Resolve issue #1954: Error on add or import of a path that is
> invalid in Subversion.
>
> ####################################################
> ### ###
> ### Note: This is a draft patch for review only, ###
> ### please do not commit it. ###
> ### ###
> ####################################################
Definitely missed this :)
> * subversion/include/svn_path.h,
> subversion/libsvn_subr/path.c
> (svn_path_is_valid_in_svn): New function.
Ah, a much nicer format than some of the 'svn log' output on trunk.
Multiple functions were not listed on separate lines.
[snip]
> (My comments will be similar to what others have said already in
> response to this patch.)
>
> Your doc string describes the implementation. Instead, it should
> describe the *semantics* -- the behavior of the function. For
> example, what happens if the path is not valid? Is an error returned,
> and if so, which error code?
OK, make sense.
> Here's a new interface, with a new doc string:
>
> /**
> * @since New in 1.2.
> *
> * Set @a *is_valid to TRUE if @a path is a valid Subversion path,
> * else set it to FALSE. Use @a pool for temporary allocation.
> *
> * Valid Subversion paths are UTF-8 strings with no control characters
> * except for SPACE (0x20). "Valid" means Subversion can store
> * it in a repository. There may be other, OS-specific limitations
> * on what paths can be represented in a working copy.
> */
> svn_error_t *svn_path_is_valid (svn_boolean_t *is_valid,
> const char *path,
> apr_pool_t *pool);
>
> (Note also that I shortened the function name.)
OK, will change.
> I'm not sure whether you need to return 'svn_error_t *' at all or not
> -- it partly depends on what you'll be calling. If it turns out that
> an error can happen, then add a paragraph describing the
> circumstances, and (ideally) the error codes.
Changing to return svn_boolean_t.
> The reason I changed the prototype is that the path being invalid is
> *not* an error :-). After all, determining whether or not a path is
> valid is exactly what function is meant do. So if it successfully
> determines that, that shouldn't be an error condition.
I'll implement the error generating function you've suggested below as a
way to raise errors and just return a TRUE/FALSE value here.
> More practically, every svn_error_t we manufacture is allocated in a
> global pool. If someone were to call your function many times on
> invalid paths, then we'd have needless memory growth.
That's good to know. I'd been proceeding on the assumption that a client
would error-exit, but just realized that a GUI client would leak memory.
[snip]
> You don't need to say "for svn_ctype_()", it's obvious from the header
> name.
OK.
> You've already seen Philip's comments about testing byte-by-byte.
> UTF-8 is a multibyte encoding. I'm not sure how familiar you are with
> it, but http://svn.red-bean.com/repos/main/3bits/utf8_xml.txt
> summarizes how it works. I'm posting it here because I've been hoping
> Brane or someone would proofread it anyway :-).
Thank you for the link, I was way off-base in my use of svn_ctype_*
functions :( Well, at least for multi-byte UTF8 characters.
> Anyway, you should use the existing svn_utf_* functions, and even
> write new ones if you need them.
Looks like I'll need to add a new public svn_utf_ function around
check_cstring_utf8 then.
> Why wrap the error in another error that contains basically the same
> information? Just "return err;" :-).
Caught me :) trying to decide between returning a boolean and an error.
> I don't think you need these comments explaining each call anyway (the
> name of the function does that quite well). But if you are going to
> put a comment, then write it in terms of semantics, not
> implementation, i.e.,
>
> /* Make sure this path is valid. */
OK.
> > + SVN_ERR (svn_io_check_path (path, &kind, pool));
>
> You appear to have added this call to svn_io_check_path(). Is it part
> of some other change? It's not mentioned in the log message.
Sorry, missed taking it out (part of attempt to check
characters at the lowest-possible level, turned out to be too
restrictive)
> Thanks! And have patience... several rounds of review are not
> unusual :-).
Thanks for the careful review. Considering the number of scattered
changes, I'm glad the code is being inspected exhaustively.
Regards
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by kf...@collab.net.
Philip Martin <ph...@codematters.co.uk> writes:
> The document is out of date. It claims UTF-16 is the simplest
> encoding and that it's fixed-width, but these days Unicode code points
> go up to 0x10FFFF and so UTF-16 is now strictly a multi-byte encoding.
> UTF-32 is probably the simplest, fixed-width encoding. However since
> the Unicode standard itself claims that nearly all characters in all
> modern languages are below 0xFFFF, treating UTF-16 as fixed-width is
> likely to work in most cases.
Thank you for the correction, Philip. I'll update the document.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by Philip Martin <ph...@codematters.co.uk>.
kfogel@collab.net writes:
> UTF-8 is a multibyte encoding. I'm not sure how familiar you are with
> it, but http://svn.red-bean.com/repos/main/3bits/utf8_xml.txt
> summarizes how it works. I'm posting it here because I've been hoping
> Brane or someone would proofread it anyway :-).
The document is out of date. It claims UTF-16 is the simplest
encoding and that it's fixed-width, but these days Unicode code points
go up to 0x10FFFF and so UTF-16 is now strictly a multi-byte encoding.
UTF-32 is probably the simplest, fixed-width encoding. However since
the Unicode standard itself claims that nearly all characters in all
modern languages are below 0xFFFF, treating UTF-16 as fixed-width is
likely to work in most cases.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by kf...@collab.net.
VK Sameer <sa...@collab.net> writes:
> First cut at issue 1954 - We should error out on pathnames added or
> imported that we don't accept.
>
> Added checks based on svn_ctype_isutf8(), svn_ctype_iscntrl().
>
> * subversion/include/svn_path.h
> subversion/libsvn_subr/path.c
> (svn_path_is_valid_in_svn): New function
>
> * General comment - added calls to svn_path_is_valid_in_svn()
> in functions listed below
>
> * subversion/libsvn_wc/adm_ops.c
> modified svn_wc_add()
>
> * subversion/libsvn_ra_local/ra_plugin.c
> modified svn_ra_local__do_check_path()
>
> * subversion/libsvn_client/copy.c
> modified wc_to_wc_copy()
>
> * subversion/libsvn_client/add.c
> modified add_dir_recursive(), add_file()
>
> * subversion/libsvn_client/commit.c
> modified import_file(), import_dir()
>
> * subversion/tests/clients/cmdline/commit_tests.py
> pulled tab test code from commit_uri_unsafe() into tab_test()
Run 'svn log' on the Subversion trunk to see examples of our usual log
formatting style. See also the section "Writing log messages" in the
HACKING file.
Here is how I would write it:
Resolve issue #1954: Error on add or import of a path that is
invalid in Subversion.
####################################################
### ###
### Note: This is a draft patch for review only, ###
### please do not commit it. ###
### ###
####################################################
* subversion/include/svn_path.h,
subversion/libsvn_subr/path.c
(svn_path_is_valid_in_svn): New function.
* subversion/libsvn_wc/adm_ops.c
(svn_wc_add): Call svn_path_is_valid_in_svn().
* subversion/libsvn_ra_local/ra_plugin.c
(svn_ra_local__do_check_path): Same.
* subversion/libsvn_client/copy.c
(wc_to_wc_copy): Same.
* subversion/libsvn_client/add.c
(add_dir_recursive, add_file): Same.
* subversion/libsvn_client/commit.c
(import_file, import_dir): Same.
* subversion/tests/clients/cmdline/commit_tests.py
(tab_test): New test.
(test_list): Run it XFail.
Okay, on to the patch itself...
> Index: subversion/include/svn_path.h
> ===================================================================
> --- subversion/include/svn_path.h (revision 12179)
> +++ subversion/include/svn_path.h (working copy)
> @@ -338,6 +338,22 @@
> svn_boolean_t svn_path_is_backpath_present (const char *path);
>
>
> +/**
> + * @since New in 1.2.
> + *
> + * Test to see if a path is allowed in a Subversion repository.
> + * First check whether all characters are valid in UTF8
> + * Next check whether all characters are non-control characters
> + * Currently, all other paths are allowed.
> + *
> + * This is OS-independent - it is used to decide what paths may be
> + * added to a repository. Adding occurs either using a file on disk
> + * (svn import/add) or one not on disk (svn mv/copy URL1 URL2).
> + */
> +svn_error_t *svn_path_is_valid_in_svn (const char *path,
> + apr_size_t len,
> + apr_pool_t *pool);
> +
(My comments will be similar to what others have said already in
response to this patch.)
Your doc string describes the implementation. Instead, it should
describe the *semantics* -- the behavior of the function. For
example, what happens if the path is not valid? Is an error returned,
and if so, which error code?
Here's a new interface, with a new doc string:
/**
* @since New in 1.2.
*
* Set @a *is_valid to TRUE if @a path is a valid Subversion path,
* else set it to FALSE. Use @a pool for temporary allocation.
*
* Valid Subversion paths are UTF-8 strings with no control characters
* except for SPACE (0x20). "Valid" means Subversion can store
* it in a repository. There may be other, OS-specific limitations
* on what paths can be represented in a working copy.
*/
svn_error_t *svn_path_is_valid (svn_boolean_t *is_valid,
const char *path,
apr_pool_t *pool);
(Note also that I shortened the function name.)
I'm not sure whether you need to return 'svn_error_t *' at all or not
-- it partly depends on what you'll be calling. If it turns out that
an error can happen, then add a paragraph describing the
circumstances, and (ideally) the error codes.
The reason I changed the prototype is that the path being invalid is
*not* an error :-). After all, determining whether or not a path is
valid is exactly what function is meant do. So if it successfully
determines that, that shouldn't be an error condition.
More practically, every svn_error_t we manufacture is allocated in a
global pool. If someone were to call your function many times on
invalid paths, then we'd have needless memory growth.
So both from an interface consistency standpoint and a performance
standpoint, returning the answer by reference as a boolean is better.
(But see below about offering a wrapper function for convenience.)
> /** Test if @a path2 is a child of @a path1.
> * If not, return @c NULL.
> * If so, return a copy of the remainder path, allocated in @a pool.
> Index: subversion/libsvn_wc/adm_ops.c
> ===================================================================
> --- subversion/libsvn_wc/adm_ops.c (revision 12179)
> +++ subversion/libsvn_wc/adm_ops.c (working copy)
> @@ -879,6 +879,10 @@
> svn_node_kind_t kind;
> apr_uint32_t modify_flags = 0;
> svn_wc_adm_access_t *adm_access;
> +
> + SVN_ERR (svn_path_is_valid_in_svn (path,
> + strlen(path),
> + pool));
Naturally, all of these calls would have to change to use some
'&boolean_var'.
But since in most call sites you just want to error anyway, it would
make sense to have a shared wrapper:
/**
* @since New in 1.2.
*
* Return SVN_ERR_FS_PATH_SYNTAX if @a path is not a valid
* Subversion path, else return SVN_NO_ERROR. See
* @c svn_path_is_valid() for more about valid paths.
*/
svn_error_t *svn_path_error_if_invalid (const char *path,
apr_pool_t *pool);
The reason to have both the return-by-reference interface and the
error-returning wrapper is so that there's a way to ask the question
without generating an error (which is useful in some circumstances)
and a way to ask it and get an error automatically (which is useful in
other circumstances).
Naturally, most of the calls in this patch are the second kind of
circumstance. But when offering a predicate API, I think it's good to
offer a non-erroring way to answer the question too.
> /* Make sure something's there. */
> SVN_ERR (svn_io_check_path (path, &kind, pool));
> Index: subversion/libsvn_subr/path.c
> ===================================================================
> --- subversion/libsvn_subr/path.c (revision 12179)
> +++ subversion/libsvn_subr/path.c (working copy)
> @@ -29,6 +29,7 @@
> #include "svn_private_config.h" /* for SVN_PATH_LOCAL_SEPARATOR */
> #include "svn_utf.h"
> #include "svn_io.h" /* for svn_io_stat() */
> +#include "svn_ctype.h" /* for svn_ctype_() */
You don't need to say "for svn_ctype_()", it's obvious from the header
name.
> /* The canonical empty path. Can this be changed? Well, change the empty
> @@ -1248,3 +1249,25 @@
> else
> return svn_utf_cstring_to_utf8 (path_utf8, path_apr, pool);
> }
> +
> +svn_error_t*
> +svn_path_is_valid_in_svn (const char *path,
> + apr_size_t len,
> + apr_pool_t *pool)
> +{
> + int i;
> + for (i = 0; i < len; i++)
> + {
> + if (!svn_ctype_isutf8(path[i]))
> + return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> + "Invalid UTF8 character in '%s'",
> + svn_path_local_style (path, pool));
> +
> + if (svn_ctype_iscntrl(path[i]))
> + return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> + "Invalid control character in '%s'",
> + svn_path_local_style (path, pool));
> + }
> +
> + return SVN_NO_ERROR;
> +}
You've already seen Philip's comments about testing byte-by-byte.
UTF-8 is a multibyte encoding. I'm not sure how familiar you are with
it, but http://svn.red-bean.com/repos/main/3bits/utf8_xml.txt
summarizes how it works. I'm posting it here because I've been hoping
Brane or someone would proofread it anyway :-).
Anyway, you should use the existing svn_utf_* functions, and even
write new ones if you need them. Then svn_path_is_valid() can just
marshal the appropriate svn_utf_* and svn_ctype_* calls.
(The signatures of the svn_utf_* and svn_ctype_* will determine
whether you still need to return 'svn_error_t *' or not.)
> Index: subversion/libsvn_ra_local/ra_plugin.c
> ===================================================================
> --- subversion/libsvn_ra_local/ra_plugin.c (revision 12179)
> +++ subversion/libsvn_ra_local/ra_plugin.c (working copy)
> @@ -687,7 +687,15 @@
> svn_ra_local__session_baton_t *sbaton = session_baton;
> svn_fs_root_t *root;
> const char *abs_path = sbaton->fs_path;
> + svn_error_t *err;
>
> + if (path && (err = svn_path_is_valid_in_svn (path,
> + strlen(path),
> + pool)))
> + return svn_error_createf (SVN_ERR_FS_PATH_SYNTAX, err,
> + _("Invalid pathname '%s'"),
> + svn_path_local_style (path, pool));
This may not matter after the rewrite, but just for the future:
Why wrap the error in another error that contains basically the same
information? Just "return err;" :-).
> --- subversion/libsvn_client/copy.c (revision 12179)
> +++ subversion/libsvn_client/copy.c (working copy)
> @@ -74,6 +74,10 @@
> svn_wc_adm_access_t *adm_access, *src_access;
> svn_error_t *err;
>
> + /* Verify path has no invalid characters */
> + SVN_ERR (svn_path_is_valid_in_svn (src_path, strlen(src_path),
> + pool));
> +
> /* Verify that SRC_PATH exists. */
> SVN_ERR (svn_io_check_path (src_path, &src_kind, pool));
> if (src_kind == svn_node_none)
> Index: subversion/libsvn_client/add.c
> ===================================================================
> --- subversion/libsvn_client/add.c (revision 12179)
> +++ subversion/libsvn_client/add.c (working copy)
> @@ -209,6 +209,11 @@
> svn_node_kind_t kind;
> svn_boolean_t is_special;
>
> + /* Check for invalid pathname characters */
> + SVN_ERR (svn_path_is_valid_in_svn (path,
> + strlen(path),
> + pool));
> +
> /* add the file */
> SVN_ERR (svn_wc_add (path, adm_access, NULL, SVN_INVALID_REVNUM,
> ctx->cancel_func, ctx->cancel_baton,
> @@ -276,6 +281,11 @@
> svn_wc_adm_access_t *dir_access;
> apr_array_header_t *ignores;
>
> + /* Check for invalid pathname characters */
> + SVN_ERR (svn_path_is_valid_in_svn (dirname,
> + strlen(dirname),
> + pool));
> +
> /* Check cancellation; note that this catches recursive calls too. */
> if (ctx->cancel_func)
> SVN_ERR (ctx->cancel_func (ctx->cancel_baton));
> Index: subversion/libsvn_client/commit.c
> ===================================================================
> --- subversion/libsvn_client/commit.c (revision 12179)
> +++ subversion/libsvn_client/commit.c (working copy)
> @@ -176,6 +176,9 @@
> svn_node_kind_t kind;
> svn_boolean_t is_special;
>
> + /* validate pathname characters */
> + SVN_ERR (svn_path_is_valid_in_svn (path, strlen (path), pool));
> +
> SVN_ERR (svn_io_check_special_path (path, &kind, &is_special, pool));
>
> if (kind == svn_node_unknown)
> @@ -276,6 +279,9 @@
> apr_hash_index_t *hi;
> apr_array_header_t *ignores;
>
> + /* validate pathname characters */
> + SVN_ERR (svn_path_is_valid_in_svn (path, strlen (path), pool));
> +
I don't think you need these comments explaining each call anyway (the
name of the function does that quite well). But if you are going to
put a comment, then write it in terms of semantics, not
implementation, i.e.,
/* Make sure this path is valid. */
instead of
/* valid pathname characters */
The latter describes exactly how the function works. People don't
need to know those details, they just need to know what it means in
the context of the calling code :-).
> SVN_ERR (svn_wc_get_default_ignores (&ignores, ctx->config, pool));
>
> SVN_ERR (svn_io_get_dirents (&dirents, path, pool));
> @@ -427,6 +433,7 @@
> apr_array_header_t *batons = NULL;
> const char *edit_path = "";
>
> + SVN_ERR (svn_io_check_path (path, &kind, pool));
> /* Get a root dir baton. We pass an invalid revnum to open_root
> to mean "base this on the youngest revision". Should we have an
> SVN_YOUNGEST_REVNUM defined for these purposes? */
You appear to have added this call to svn_io_check_path(). Is it part
of some other change? It's not mentioned in the log message.
I haven't thought carefully about how to write the regression tests
yet, so no comment on that part of the change for now.
I also haven't thought carefully about the call sites. When the next
revision of the patch comes in, I'll look at those in more detail.
Thanks! And have patience... several rounds of review are not
unusual :-).
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by kf...@collab.net.
Branko Čibej <br...@xbc.nu> writes:
> >From now on, I will use the phrase "UTF-8 character" with this
> >meaning. I may even use that language in a Subversion doc string
> >someday. But I will not use it in an error message without bicameral
> >approval from the Slovenian parliament.
> >
> You'll wait 'till forever for that, we've a unicameral setup. :-)
Can you fix Wikipedia, then?
http://en.wikipedia.org/wiki/Slovenia
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by Branko Čibej <br...@xbc.nu>.
kfogel@collab.net wrote:
From now on, I will use the phrase "UTF-8 character" with this
>meaning. I may even use that language in a Subversion doc string
>someday. But I will not use it in an error message without bicameral
>approval from the Slovenian parliament.
>
>
You'll wait 'till forever for that, we've a unicameral setup. :-)
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by kf...@collab.net.
Greg Stein <gs...@lyra.org> writes:
> > A series of 1 to 4 bytes that represents a Unicode code point,
> > using the encoding described by the UTF-8 specification.
>
> UTF-8 is an encoding rather than a character set. Thus, it does not
> define any characters. *Unicode* is a character set, and UTF-8 is a
> particularing encoding of that charset. Therefore, Brane is right:
> UTF-8 does not define any characters -- Unicode does.
>
> Branko's suggested text is "closer to reality". There is an error in
> the encoding, rather than an erroneous character.
>
> All that said: given that UTF-8 only applies to the Unicode charset,
> there is a very fine line there. But to be pedantic... :-)
Heh heh! Nice. Okay, I guess if we're really going to pick nits,
then Brane is right :-).
Ahem.
I hereby define a UTF-8 character to be [see above-quoted definition].
Re: [PATCH] First cut at 1954 solution
Posted by Greg Stein <gs...@lyra.org>.
On Wed, Dec 08, 2004 at 12:27:17AM -0600, kfogel@collab.net wrote:
> VK Sameer <sa...@collab.net> writes:
> > On Wed, 2004-12-08 at 03:26, Branko ??ibej wrote:
>...
> > > Hmmm. "Invalid byte in UTF-8 sequence" would be closer to the mark;
> > > there's no such thing as an UTF-8 character.
> >
> > RFC3629 (http://www.ietf.org/rfc/rfc3629.txt) uses that phrase:
> >
> > "Decoding a UTF-8 character proceeds as follows:"
>
> Yeah, I also wondered what Brane meant by the assertion that there is
> no such thing as a UTF-8 character :-). I assumed the definition was
> the obvious one:
>
> A series of 1 to 4 bytes that represents a Unicode code point,
> using the encoding described by the UTF-8 specification.
UTF-8 is an encoding rather than a character set. Thus, it does not
define any characters. *Unicode* is a character set, and UTF-8 is a
particularing encoding of that charset. Therefore, Brane is right:
UTF-8 does not define any characters -- Unicode does.
Branko's suggested text is "closer to reality". There is an error in
the encoding, rather than an erroneous character.
All that said: given that UTF-8 only applies to the Unicode charset,
there is a very fine line there. But to be pedantic... :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by kf...@collab.net.
VK Sameer <sa...@collab.net> writes:
> On Wed, 2004-12-08 at 03:26, Branko Čibej wrote:
> > VK Sameer wrote:
> >
> > >+ if (!svn_ctype_isutf8(path[i]))
> > >+ return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> > >+ "Invalid UTF8 character in '%s'",
> > >+ svn_path_local_style (path, pool));
> > >
> > Hmmm. "Invalid byte in UTF-8 sequence" would be closer to the mark;
> > there's no such thing as an UTF-8 character.
>
> RFC3629 (http://www.ietf.org/rfc/rfc3629.txt) uses that phrase:
>
> "Decoding a UTF-8 character proceeds as follows:"
Yeah, I also wondered what Brane meant by the assertion that there is
no such thing as a UTF-8 character :-). I assumed the definition was
the obvious one:
A series of 1 to 4 bytes that represents a Unicode code point,
using the encoding described by the UTF-8 specification.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by Branko Čibej <br...@xbc.nu>.
VK Sameer wrote:
>On Wed, 2004-12-08 at 03:26, Branko Čibej wrote:
>
>
>>VK Sameer wrote:
>>
>>
>>
>>>+ if (!svn_ctype_isutf8(path[i]))
>>>+ return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
>>>+ "Invalid UTF8 character in '%s'",
>>>+ svn_path_local_style (path, pool));
>>>
>>>
>>>
>>Hmmm. "Invalid byte in UTF-8 sequence" would be closer to the mark;
>>there's no such thing as an UTF-8 character.
>>
>>
>
>RFC3629 (http://www.ietf.org/rfc/rfc3629.txt) uses that phrase:
>
> "Decoding a UTF-8 character proceeds as follows:"
>
>Also, instead of a byte-by-byte check for invalid UTF-8, I'll be using
>svn_utf_check_cstring_utf8() (wrapper around check_cstring_utf8). It
>calls invalid_utf8() to put the offending bytes (in hex) in the error
>string.
>
>
O.K.
>>Although I can't imagine how we'd display that message if
>>"path" isn't valid UTF-8, since the output conversion will fail.
>>
>>
>
>Yes, sprintf turned control characters to spaces. I was hoping the
>surrounding, hopefully valid, characters would let the user figure out
>the offending pathname.
>
>
I wonder how easy it is to /find/ the surrounding valid characters;
those before the error are obvious, those after not quite so. But I
suppose we don't have to be 100% correct here anyway.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by VK Sameer <sa...@collab.net>.
On Wed, 2004-12-08 at 03:26, Branko Čibej wrote:
> VK Sameer wrote:
>
> >+ if (!svn_ctype_isutf8(path[i]))
> >+ return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
> >+ "Invalid UTF8 character in '%s'",
> >+ svn_path_local_style (path, pool));
> >
> Hmmm. "Invalid byte in UTF-8 sequence" would be closer to the mark;
> there's no such thing as an UTF-8 character.
RFC3629 (http://www.ietf.org/rfc/rfc3629.txt) uses that phrase:
"Decoding a UTF-8 character proceeds as follows:"
Also, instead of a byte-by-byte check for invalid UTF-8, I'll be using
svn_utf_check_cstring_utf8() (wrapper around check_cstring_utf8). It
calls invalid_utf8() to put the offending bytes (in hex) in the error
string.
> Although I can't imagine how we'd display that message if
> "path" isn't valid UTF-8, since the output conversion will fail.
Yes, sprintf turned control characters to spaces. I was hoping the
surrounding, hopefully valid, characters would let the user figure out
the offending pathname.
Sameer
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by Branko Čibej <br...@xbc.nu>.
VK Sameer wrote:
>+ if (!svn_ctype_isutf8(path[i]))
>+ return svn_error_createf(SVN_ERR_FS_PATH_SYNTAX, NULL,
>+ "Invalid UTF8 character in '%s'",
>+ svn_path_local_style (path, pool));
>
>
Hmmm. "Invalid byte in UTF-8 sequence" would be closer to the mark;
there's no such thing as an UTF-8 character. Although I can't imagine
how we'd display that message if "path" isn't valid UTF-8, since the
output conversion will fail.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] First cut at 1954 solution
Posted by Branko Čibej <br...@xbc.nu>.
VK Sameer wrote:
>+svn_error_t*
>+svn_path_is_valid_in_svn (const char *path,
>+ apr_size_t len,
>+ apr_pool_t *pool)
>+{
>+ int i;
>+ for (i = 0; i < len; i++)
>+ {
>
Signed/unsigned mismatch here. Never use a signed induction variable
when the loop limit is unsigned.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org