[PATCH] Put quotes around branch names to prevent special characters from being interpreted by the shell.

[PATCH] Put quotes around branch names to prevent special characters from being interpreted by the shell.

[Benjamin C Meyer]

Perforce branch names with spaces (and other special characters) were causing issues.

Signed-off-by: Benjamin C Meyer <bmeyer@rim.com>
---
 contrib/fast-import/git-p4 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index c1ea643..d42b865 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1228,7 +1228,7 @@ class P4Sync(Command):
         lostAndFoundBranches = set()
 
         for info in p4CmdList("branches"):
-            details = p4Cmd("branch -o %s" % info["branch"])
+            details = p4Cmd("branch -o \"%s\"" % info["branch"])
             viewIdx = 0
             while details.has_key("View%s" % viewIdx):
                 paths = details["View%s" % viewIdx].split(" ")
-- 
1.7.1

Re: [PATCH] Put quotes around branch names to prevent special characters from being interpreted by the shell.

[Jeff King]

On Fri, Jun 04, 2010 at 11:53:20AM -0400, Benjamin C Meyer wrote:

> Perforce branch names with spaces (and other special characters) were
> causing issues.
> [...]
> -            details = p4Cmd("branch -o %s" % info["branch"])
> +            details = p4Cmd("branch -o \"%s\"" % info["branch"])

Won't this still fail if your branch name contains quotation marks or
backslashes? Does python have some equivalent to perl's quotemeta to
just shell-escape the problematic characters (or even a way of just
using a straight list in p4Cmd and avoiding the shell altogether)?

I know those characters may not be common, but if we are going to quote,
perhaps we should make it foolproof.

-Peff

Re: [PATCH] Put quotes around branch names to prevent special characters from being interpreted by the shell.

[Jay Soffian]

On Sun, Jun 6, 2010 at 5:55 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jun 04, 2010 at 11:53:20AM -0400, Benjamin C Meyer wrote:
>
>> Perforce branch names with spaces (and other special characters) were
>> causing issues.
>> [...]
>> -            details = p4Cmd("branch -o %s" % info["branch"])
>> +            details = p4Cmd("branch -o \"%s\"" % info["branch"])
>
> Won't this still fail if your branch name contains quotation marks or
> backslashes? Does python have some equivalent to perl's quotemeta to
> just shell-escape the problematic characters

from commands import mkarg
mkarg(arg)

will enclose arg in single quotes if it contains none; else it will
enclose arg in double quotes and backslash escape shell meta
characters (any of \ $ " `).

BTW, quotemeta is technically intended for use with regular
expressions, isn't it?

> (or even a way of just
> using a straight list in p4Cmd and avoiding the shell altogether)?

The subprocess module.

> I know those characters may not be common, but if we are going to quote,
> perhaps we should make it foolproof.

+1.

j.

Re: [PATCH] Put quotes around branch names to prevent special characters from being interpreted by the shell.

[Ævar Arnfjörð Bjarmason]

On Mon, Jun 7, 2010 at 05:10, Jay Soffian <jaysoffian@gmail.com> wrote:
> BTW, quotemeta is technically intended for use with regular
> expressions, isn't it?

Yes, it's completely insecure to use it for shell interpolation.

In Perl it's best to use the list form of system() so that the command
will escape things for you automatically.

Re: [PATCH] Put quotes around branch names to prevent special characters from being interpreted by the shell.

[Ian Ward Comfort]

On 6 Jun 2010, at 10:48 PM, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Jun 7, 2010 at 05:10, Jay Soffian <jaysoffian@gmail.com>  
> wrote:
>> BTW, quotemeta is technically intended for use with regular  
>> expressions, isn't it?
>
> Yes, it's completely insecure to use it for shell interpolation.
>
> In Perl it's best to use the list form of system() so that the  
> command will escape things for you automatically.

Passing a list to system() actually ensures that Perl will call execvp  
directly, instead of looking for metacharacters and possibly invoking  
the system shell.  (But this is getting a little OT; sorry.)

-- 
Ian Ward Comfort <icomfort@stanford.edu>
Systems Team Lead, Academic Computing Services, Stanford University

Re: [PATCH] Put quotes around branch names to prevent special characters from being interpreted by the shell.

[Jeff King]

On Mon, Jun 07, 2010 at 05:48:11AM +0000, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Jun 7, 2010 at 05:10, Jay Soffian <jaysoffian@gmail.com> wrote:
> > BTW, quotemeta is technically intended for use with regular
> > expressions, isn't it?
> 
> Yes, it's completely insecure to use it for shell interpolation.

It's intended for use with regexps, but I don't think it is insecure for
shell interpolation. According to perldoc, it quotes 'all characters not
matching "/[A-Za-z_0-9]/"'. So it's excessive for shell quoting, but not
insecure.

> In Perl it's best to use the list form of system() so that the command
> will escape things for you automatically.

Agreed, but it's not escaping things automatically. It simply skips the
shell invocation entirely.

-Peff