Karl Fogel <kfogel@newton.ch.collab.net> writes:
> Zack Weinberg <zack@codesourcery.com> writes:
> > you've missed the point. The chdir to the WC base has nothing to do
> > with possible OS-level access problems. It's done because we have no
> > other portable way to ensure that the file name passed to the external
> > editor is not misinterpreted by the user's shell. Consider
> >
> > svnadmin create repos
> > svn co file://`pwd`/repos "My Documents"
> > touch "My Documents/2002 Proposal.doc"
> > svn add "My Documents/2002 Proposal.doc"
> > svn commit "My Documents"
> >
> > A space character is actually one of the easiest cases to deal with;
> > the nasty ones are !`'"\ etc. SVN must assume that any single byte
> > could appear in a file path, except 0x00.
> >
> > Not invoking the shell is also not an option, people expect to be able
> > to embed shell constructs in EDITOR.
>
> Are you sure about this, Zack?
>
> Although I don't remember everything about that discussion of long
> ago, my understanding was that we simply decided to punt on this
> problem. If people commit files with funny names, they should use -F
> or -m to pass the log message.
>
> In any case, I don't see how our current behavior (before Alexis's
> patch) would help solve this problem. The editor is still invoked on
> possibly dangerous file names. Okay, I guess at least intermediate
> directory names are not involved, so there's a slight gain, but that
> hardly seems sufficient to have been our motivation for cd'ing to the
> anchor directory.
There seems to be some need for clarification here since both Karl and
Zack are apparently confused as to the purpose of this excercise.
Karl: Sorry, but your analysis is incorrect; the editor is _not_
invoked on possibly dangerous file names. It is invoked on file names
of the format "msg.63416.00001.tmp". The name is generated internally
by Subversion, so it will always have this format. It can only be
made dangerous by adding intermediate directory names to it. The name
of the committed file has nothing to do with anything.
Zack: No, I don't think Alexis has missed the point at all. This has
everything to do with access problems, although in a rather roundabout
way. What the code _originally_ (before rev 2870) did was
A) create the logmsg file in /path/to/commit/base/msg.nnnn.tmp, and
run the editor on "/path/to/commit/base/msg.nnnn.tmp" in ./
After my patch, it instead did
B) create the logmsg file in /path/to/commit/base/msg.nnnn.tmp, and
run the editor on "msg.nnnn.tmp" in /path/to/commit/base/
which fixed the problem with the editor argument possibly containing
unsafe characters, but can confuse the user by running the editor in
an unexpected directory. So what Alexis patch does is basically
C) create the logmsg file in ./msg.nnnn.tmp, and run the editor on
"msg.nnnn.tmp" in ./
which is safe and intuitive. So why didn't the code do C right from
the start? Because . could be anything (like "/" for example), so it
might not be writable. The choice was therefore to store the file in
the WC, which is "known" to be writable. So in order not to have a
regression here, you have to do handle the case where C) gives you an
access problem, and fall back to B). And that's precisely what Alexis
patch is all about.
> If we really think this is a problem, we could write a function
> svn_path_dangerous_p() that simply returns true if a path is "funny"
> in the client environment (perhaps it should be apr_path_dangerous_p,
> but whatever). When any such path is involved in the commit, we'd
> refuse to pop up an editor, and insist that -F or -m be used instead.
That sounds horrible. Any code that needs to know whether a path is
"dangerous" should simply assume that all paths are "dangerous".
// Marcus
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Wed Aug 28 21:21:02 2002