On Mon, 2004-12-06 at 19:48, C. Michael Pilato wrote:
> VK Sameer <sameer@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
Received on Tue Dec 7 03:09:56 2004